Python program for testing the Collatz Conjecture This is a simple program to test if integers will follow an interesting, improvable statement that if disproven, calls for a bravo.

But, ignoring the pretentious poetry, this code allows the user to input a number, which, if it is a finite, positive integer, will be calculated according to the Collatz Conjecture as stated/commented out below. Any optimizations? Obvious mistakes? Formatting issues?

Maybe let me know how to inform the user that the function-ception went a few levels too deep?

# The Collatz conjecture states that
# when you take a finite, positive integer, and
# if it's even, divide by 2, or
# if it's odd, multiply by 3 and add 1,
# and repeat this process enough times,
# you will eventually end up with one.
# Here's a module to demonstrate it.

def collatz(x):
  if abs(int(x)) == x:
    if x == 1:
      print ("The Collatz Conjecture works with this number!")
      print ("It took %s repetitions to reach 1.") % (collatz.counter)
    elif x % 2 == 0:
      y = x / 2
      print ("%s") % (y)
      collatz.counter += 1
      collatz(y)
    elif x % 2 == 1:
      y = x * 3 + 1
      print ("%s") % (y)
      collatz.counter += 1
      collatz(y)
    else:
      print ("Oh, my god!")
      print ("This number does NOT work with the Collatz Conjecture!")
      print ("You've disproven it!")
      print ("...Or maybe this program is broken. Try viewing the source?")
  else:
    print ("That isn't a finite positive integer...")

collatz.counter = 0

#Here's our attempt:

collatz(int(raw_input("Enter a finite positive integer")))

"Oh, my god!" will only be reached if the number is neither odd nor even, which will never be the case for any integer.

For a number that will never reach 1 and thus disprove the conjecture, your program would run forever (yet still never reach "Oh, my god!"). Welcome to the halting problem.

But as computer resources are limited, you will run into a stack overflow with the depth of your recursive function calls, whether the number would reach 1 eventually or not. For very large numbers, your program will simply crash, which won't tell you anything about the conjecture.

One way to solve that would be to not use recursion. To do so, return the result y from your function instead of calling the function again. Then call your function in a while loop that stops at 1, feeding the output of a previous call to your function to the next one.

This is still the halting problem, though. Does the computation just take very long or does it never finish (the latter would disprove the conjecture)? You never know.

To solve that too, you'd have to check if your program is trapped in a cycle of numbers (that does not contain 1)


Wrapper classes for objects returned by 3rd party APIs I am building a WPF application which uses a third party library to fetch objects which I want to allow the user to interact with.

The objects are returned as interface types. Am I right in thinking it is good practise to create wrapper classes for these objects instead of binding directly to the interface returned e.g (ICustomer)?

If so, what is the best way to do this? My current thinking is to inject the service into my view model. Then create my own version of the customer class (ClientCustomer) on the client. When I call get customers on the service it will return a list of ICustomer.

For each of these, I will create a new instance of ClientCustomer passing in the ICustomer object and client customer can simply return the underlying properties. This will leave me with a list of ClientCustomer objects which I can bind to instead of binding directly to the List of ICustomer:

public class ClientCustomer
{
  public ClientCustomer(ICustomer c)
  {
    this.customer = c;
  }

  public string Name
  {
    get { return this.customer.Name;}
  }

  //Other properties in a similar way to Name
}

public class CustomerViewViewModel : ViewModelBase
{
  ICustomerService svc;

  public ViewModel(ICustomerService svc)
  {
    this.svc = svc;
    Customers = new List<ClientCustomer>();
    var custs = svc.GetCustomers();
    custs.ForEach(c => Customers.Add( new ClientCustomer(c));
  }

 public List<ClientCustomer> Customers {get;}
}

Is this a good approach?

I'm also unsure how to handle complex types which ICustomer contains. For instance ICustomer has a List of Bank accounts IBankAccount.
Do I need to wrap the Bank Account object in a similar way as well?

You're almost there. You have the right idea, but you missed the execution of the idea a little bit.

public class ClientCustomer
{
  public ClientCustomer(ICustomer c)
  {
    this.customer = c;
  }

  public string Name

You've put a boundary in between yourself and the third party interface, but then you've turned around and coded against an implementation. We should code against interfaces whenever possible.

Right now you're probably confused.

  But I can't code against that interface. I need to insulate myself from it.

You're right. You do need to insulate yourself from the third party interface. It's their boundary, not yours. You need the Adapter pattern.

You need to create your own interface that your code will work with.

interface IClientCustomer
{
    string Name { get; }
}

Then your class implements your interface.

public class ClientCustomer : IClientCustomer

And all of the client code works with classes implementing the IClientCustomer interface.

  public ViewModel(ICustomerService svc)
  {
    this.svc = svc;
    Customers = new List<IClientCustomer>();

This way, if you ever swap the libraries out, you're truly insulated. You create a new class that implements your interface and it is much easier to swap one for another. If you make this small change, I'd say you've got it right.


Rod Cutting Problem Code I am learning dynamic programming. I read articles for the rod cutting problem and I applied it in cpp, and I got a doubt, does the below code solves the problem through memoization (say, previously rod(3) was solved, then if same function was called with same parameters) has the answer, or will it use tabulation (rodMax array), or uses both?

Since I read two different articles explaining different methods(memoization and tabulation, and read as two different methods to solve a dynamic problem), kind of confused on which method the below code runs.

#include <iostream>

using namespace std;

int max(int a, int b){

    if(a > b)
        return a;

    return b;
}

int arr[5] = {2, 4, 1, 2, 7};

int rodMax[5] = {-1, -1, -1, -1, -1};

int rod(int n){

    if(n <=0)
        return 0;

    if(rodMax[n - 1] != -1){
        return rodMax[n];
    }

    int max_val = -1;
    for(int i = 0; i < n; i++){

        max_val = max(max_val, arr[n-i-1]+rod(n-1));
    }
    //cout<<"\nmax "<<max_val<<endl;
    rodMax[n - 1] = max_val;

    return max_val;
}

int main() {

    cout<<"ans "<<rod(5)<<endl;
    //cout<<"ans "<<rod(3)<<endl;
    //for(int i = 0; i <5; i++)
    //  cout<<rodMax[i]<<" ";
}
```

Don't using namespace std - especially if you're going to define a max() function in the global namespace (why not just use std::max() anyway?  That's what it's for!).

Where do the values in arr and rodMax come from?  Do their lengths have to agree?  If so, make that obvious in the code (e.g. use std::array, so that we can use arr.size() to count the members).


Showing a "back-to-top"-button after scrolling a certain amount I have a "back to top"-button on the page, It should be visible after the user has scrolled a certain amount*.

This means I have to add or remove a class active every now and then.

I have two solutions:

Update the class on every scroll.
Test whether the class is set/ not et and then test, whether it is necessary to interact.

The setup:

this.button = document.querySelector('.some-class');

window.addEventListener('scroll', event => {
  this.onScroll(event);
});

The long version, which does add or remove the class only if needed:

onScroll(event) {
  if (
    !this.button.classList.contains('active') &&
    (document.body.scrollTop > 100 || document.documentElement.scrollTop > 100)
  ) {
    this.button.classList.add('active');
  }

  if (
    this.button.classList.contains('active') &&
    (document.body.scrollTop <= 100 || document.documentElement.scrollTop <= 100)
  ) {
    this.button.classList.remove('is-active');
  }
}

The alternativ short version, that adds or removes the class everytime:

onScroll(event) {
  if (document.body.scrollTop > 100 || document.documentElement.scrollTop > 100) {
    this.button.classList.add('active');
  } else {
    this.button.classList.remove('active');
  }
}

My questions are:

Does the longer version have a better performance?
Does the browser optimize it itself – i.e. in the short version if the class is already present, it doesn't repaint?

* For simplicity this value is hard-coded here. It's a variable in the implemented version.

Does the longer version have a better performance?

No, the longer version has two different if conditions and you're recalculating the scroll area twice (i.e.: document.body.scrollTop > 100 || document.documentElement.scrollTop > 100 and document.body.scrollTop <= 100 || document.documentElement.scrollTop <= 100)

  Does the browser optimize it itself – i.e. in the short version if the class is already present, it doesn't repaint?

Yes, if the class name already exists it'll simply be ignored.

classList MDN Documentation:

  add( String [, String [, ...]] )

  Adds the specified class values. If these classes already exist in the element's class attribute they are ignored.

Handling the scroll event can be performant heavy if you have a complex handler as the scroll event will be continuously fired during a scroll.

If you wish to optimize your scroll event handler, I would highly recommend using setTimeout. This allows your script to ONLY be called once the scroll has ended and skips doing your "heavy" condition checking for every scroll tick.

let setTimeoutId = null;
window.addEventListener('scroll', event => {
  //clear a previously pending timeout
  clearTimeout(setTimeoutId);
  //create a new timeout that will be launched in 400ms
  setTimeoutId = setTimeout(()=>onScroll(event), 100);
});

function onScroll(event){
  console.log("On scroll called");
}
body {
  height: 2000px;
  background-color: lightblue;
}


Processing large file in Python I have some code that calculates the "sentiment" of a Tweet.

The task starts with an AFINN file, that is a tab-separated list of around 2500 key-value pairs. I read this into a dict using the following function:

csv.register_dialect('tsv', delimiter='\t')

def processSentiments(file):
    with open(file) as sentiments:
        reader = csv.reader(sentiments,'tsv')
        return dict((word, int(sentiment)) for word, sentiment in reader)

I'm happy with this code - it's pretty clear and I'm not worried about its speed and memory usage as the file is small.

Using the created dict I now need to process many tweets. Lets say I have 1GB of then in JSON format - from the live stream.

What I need to do is:

read a tweet file, with a JSON tweet on each line
parse each tweet to a dict using json.loads
extract the text field from the tweet - giving the content of the tweet
for each word in the content, check it if has a sentiment
for each sentiment word in the tweet, calculate it's value (from the AFINN dict) and sum across the tweet
store that number

So whilst I have a lot of tweet data, I only need a list of integers. This is obviously much smaller.

I want to be able to stream the data from the file and convert each line to its sentiment value in the most efficient way possible. Here's what I came up with:

def extractSentiment(tweetText, sentimentsDict):
    return sum([sentimentsDict.get(word, 0) for word in tweetText.split()])

def processTweet(tweet, sentimentsDict):
    try:
        tweetJson = json.loads(tweet)
    except ValueError, e:
        raise Exception('Invalid tweet: ', tweet, e)
    tweetText = tweetJson.get('text', False)
    if tweetText:
        return extractSentiment(tweetText, sentimentsDict)
    else:
        return 0

def processTweets(file, sentimentsDict):
    with open(file) as tweetFile:
        return [processTweet(tweet, sentimentsDict) for tweet in tweetFile]

So I am using a for-comprehension over the tweetsfile to map each line in the file to its corresponding number.

So my question is, how would you rewrite the above the maximise speed and minimise memory usage.

Any other comments on the Python would be welcome too.

I realise that I can use a single-producer -> multiple-consumer pattern to speed up the processing of the file, but I would for now like to stick with a single thread.

As you may have probably suspected, the list comprehension in processTweets makes this non-streaming and eat a lot of memory, as it has to contain the entire result dictionary in memory before returning to the caller. Which might be fine, as it's just a list of integers.

You can make this streaming by turning this method into a generator:

def processTweets(path, sentimentsDict):
    with open(path) as fh:
        for tweet in fh:
            yield processTweet(tweet, sentimentsDict)

Note that I renamed your variables (file -> path, tweetFile -> fh, because file shadows an existing Python class name (and usually syntax highlighted), and because I like to use fh for throwaway filehandles ;-)

You don't need the default False value in tweetJson.get('text', False), this will work just fine, because the .get will return None, which is falsy:

tweetText = tweetJson.get('text')
if tweetText:
    return extractSentiment(tweetText, sentimentsDict)
else:
    return 0

In processTweet, you catch ValueError and raise an Exception. Since you are not doing anything special to handle the ValueError, you could just let the error bubble up, and it might speed up the process a bit if Python doesn't need to wrap the code within a try-catch for every single tweet.


Sort int array by frequency then value Problem

Take an array of ints as input and run a special sorting algorithm on it. This algorithm must first sort by int frequency, less frequent first, then sort by value.

For example [4,5,6,5,4,3] is sorted to [3,6,4,4,5,5].

I'm looking for feedback on my use of data structures to implement this sort, memory management, any redundancies, and other general optimizations I can make. No need to give feedback on how I capture input in main since that is not my priority here.

Code

#include <map>
#include <set>
#include <list>
#include <cmath>
#include <ctime>
#include <deque>
#include <queue>
#include <stack>
#include <string>
#include <bitset>
#include <cstdio>
#include <limits>
#include <vector>
#include <climits>
#include <cstring>
#include <cstdlib>
#include <fstream>
#include <numeric>
#include <sstream>
#include <iostream>
#include <algorithm>
#include <unordered_map>

using namespace std;

int * customSort(int * arr, int n) {
  unordered_map<int, int> frequencyMap;
  for (int i = 0; i < n; i++) {
    auto mapIt = frequencyMap.find(arr[i]);
    if (mapIt == frequencyMap.end()) {
      frequencyMap.insert(make_pair(arr[i], 1));
    } else {
      frequencyMap[arr[i]]++;
    }
  }

  map<int, set<int>> orderedfrequencyMap;
  for (auto &p : frequencyMap) {
    auto mapIt = orderedfrequencyMap.find(p.second);
    if (mapIt == orderedfrequencyMap.end()) {
      set<int> s = {p.first};
      orderedfrequencyMap.insert(make_pair(p.second, s));
    } else {
      (*mapIt).second.insert(p.first);
    }
  }

  int arrI = 0;
  int solution[n];
  for (auto &p : orderedfrequencyMap) {
    for (auto element : p.second) {
      for (int i = 0; i < p.first; i++) {
        arr[arrI] = element;
        cout << element << endl;
        arrI++;
      }
    }
  }

  return solution;
}

int main() {
    int n;
    cin >> n;

    int arr[n];

    int element;
    for (int i = 0; i < n; i++) {
      cin >> element;
      arr[i] = element;
    }

    customSort(arr, n);

    return 0;
}

Headers

#include <map>
#include <set>
#include <list>
#include <cmath>
#include <ctime>
#include <deque>
#include <queue>
#include <stack>
#include <string>
#include <bitset>
#include <cstdio>
#include <limits>
#include <vector>
#include <climits>
#include <cstring>
#include <cstdlib>
#include <fstream>
#include <numeric>
#include <sstream>
#include <iostream>
#include <algorithm>
#include <unordered_map>

Some issues here. You definitely don't need all those headers. Proon out the ones you don't need. The problem with putting them all in is that this can hide missing dependencies (if this was a header file and included by other header files).

Normally I don't care much about the order of headers (usually there is some logic there and everybody has their own). But sorting by filename length of include seems a bit odd. I have seen people sort by alphabetical order (not my favorite but it has options and I see the merit). Personally I put things into logical (to me) groups; I put container stuff together, stream stuff together, algorithm stuff together and meta programming stuff together. I put all my C++ headers before my C header.

Maps

If you access an element in a map and it does not exist then it is automatically created and "zero-initialized". This means you don't need to search and conditialy update.

  unordered_map<int, int> frequencyMap;
  for (int i = 0; i < n; i++) {
    auto mapIt = frequencyMap.find(arr[i]);
    if (mapIt == frequencyMap.end()) {
      frequencyMap.insert(make_pair(arr[i], 1));
    } else {
      frequencyMap[arr[i]]++;
    }
  }

Can be simplified too:

  unordered_map<int, int> frequencyMap;
  for (int i = 0; i < n; i++) {
      ++frequencyMap[arr[i]];
  }

VLA

Variable length arrays are an extension of C (part of C99). They were never part of the C++ standard. So this is not legal C++.

cin >> n;
int arr[n];

Your compiler supports it as an extension to the language but its not a good idea to use it. We use std::vector as a better alternative:

cin >> n;
std::vector<int> arr(n);

This also allows you to use range based for loops:

  unordered_map<int, int> frequencyMap;
  for (int val: arr) {
      ++frequencyMap[val];
  }

Order by frequency.

I like your idea of ordering by frequency using a std::map but it becomes clunky when you also have to order by value (and thus have a map of std::set). I would simply copy the frequency info into a vector and use a sort.

using Val = std::pair<int, int>;
std::vector<Val>  values(std::begin(frequencyMap), std::end(frequencyMap));
std::sort(std::begin(values), std::end(values), [](Val const& lhs, Val const& rhs)
  {
    return (lhs.second < rhs.second) || (lhs.second == rhs.second && lhs.first < rhs.first);
  });

Standard Issues from Beginners

Don't use

using namespace std;

This is a crutch that will break for you. Don't get into the habit of using it when it breaks it is not obvious and can lead to hours of debugging. The reason the standard namespace is called std and not standard is so prefixing object in the standard namespace is not that burdensome.

std::vector<int>  val;  // not that hard to add std::

Don't return an array from a function

 return solution;

You are returning the address of a local variable. Once this goes out of scope that variable no longer exists. So you have an invalid pointer returned. This is also another good reason to use std::vector as the copy/move semantics will correctly move it from the current scope to the calling scope.

PS. Your compiler should warn you about this.

No need to use return in main

return 0;

The compiler plants a return 0 at the end of main.

By not planting a return in main() it is an indication that your application does not have a failing state (usual for simple code). If it does have a user inserted return 0 I always start looking for other places it could return an error code. By not putting a return 0 you indicate to me that there is no error situation and I don't have to look for it.

Prefer pre increment to post increment

for (int i = 0; i < p.first; i++)

for (int i = 0; i < p.first; ++i)

In this case there is basically no difference. But there are situations where the post increment is slightly more complex.

What happen if we change i from an int into an iterator. Then it is a class the executes a function. Most iteratros behave like POD types with pre/post increment but to do that the post increment is slightly more complex. So why not use the one that will always give you optimal code (even if you change the type). So prefer pre-increment.

Prefer '\n' over std::endl

The difference is that std::endl will force a flush. In simple things like this it makes no difference. But if you are outputting a large amount of information then the extra flush can cause significant performance degradation.

The streams will auto flush themselves and having a human forcing a flush is always non optimal. So prefer to get the same resutls but at optimal speed with '\n'.

Pointer short hand

You should note that we have a shortcut pointer notation.

(*mapIt).second

// Shorthand as:
mapIt->second

Avoid short variable names

for (int i = 0; i < n; i++) {

This is great if your for loop is two lines long. But if the loop grows and becomes more complex then searching for all instances of i becomes a pain because of all the false positives.

Believe me just because your loop only covers two lines today, in ten years after thousands of bug fixes it will not look as neat. So give me something that is easy to search for in the code base.

I know a lot of people like using i as a loop variable. But this is a hold over from Foortran. Please use longer identifiers it will not cost you that much and the maintainer will not hunt you down with an axe.

Looks Like this:

#include <unordered_map>
#include <vector>
#include <iostream>
#include <iterator>

std::vector<int> customSort(std::vector<int> const& arr)
{
    std::unordered_map<int, int> frequencyMap;
    for (int val: arr) {
        ++frequencyMap[val];
    }

    using Val = std::pair<int, int>;
    std::vector<Val>  values(std::begin(frequencyMap), std::end(frequencyMap));
    std::sort(std::begin(values), std::end(values), [](Val const& lhs, Val const& rhs) {
            return (lhs.second < rhs.second) || (lhs.second == rhs.second && lhs.first < rhs.first);
          });

    std::vector<int> result;
    result.reserve(arr.size());
    for(auto const& val: values) {
        for(int loop = 0;loop < val.second; ++loop) {
            result.push_back(val.first);
        }
    }
    return result;
}

int main()
{
    std::vector<int> arr;

    int n;
    std::cin >> n;
    std::copy_n(std::istream_iterator<int>(std::cin), n,
            std::back_inserter(arr));

    std::vector<int> sorted = customSort(arr);
    std::copy(std::begin(sorted), std::end(sorted),
              std::ostream_iterator<int>(std::cout, "\n"));
}


Calculate the difference between current date and age from personal ID The goal is to show message for users that are less 25 years old. The personal ID consists of 081010333333, where 08year, 10month, 10date. I made code and it's very messy. So the question is: how to make it "right"? any advices and solutions are welcome. One on codepen and the code itself:

$('input').on('change keyup', function(i, el){
    var d = new Date(),
            getVal = $(this).val(),
            getYear = $(this).val().slice(0,2),
            getMonth = $(this).val().slice(2,4),
            getDay = $(this).val().slice(4,6),
            getCurrentYearFull = d.getFullYear(),
            getCurrentMonth = ("0" + (d.getMonth() + 1)).slice(-2),
            getCurrentDate = ("0" + (d.getDate())).slice(-2),
            getCurrentYearShort = d.getFullYear().toString().slice(2,4);

        if(getYear > getCurrentYearShort){
                getYear = "19" + getYear
            } else {
                getYear = "20" + getYear
            }
            var yearDiff = getCurrentYearFull - getYear;
            if (yearDiff >=10 && $('input').val().length >= 4) {
                    if(getCurrentMonth >= getMonth){
                         if (getCurrentDate >= getDay){
                             $(".check").text('good')
                         }
                        else {
                            $(".check").text('bad')
                        }
                    } else {
                        $(".check").text('bad')
                    }
            } else {
                $(".check").text('bad')

            }
        $('.test').text(yearDiff)
})

<input type="text" value="120330"/>

In this answer I'll just reveiw the way you calculate the age difference.
So first, I would like to mention, that you should really work with four-digit numbers for displaing years - if that's possible for you.

In the suggestion below you'll see, that I don't compare the years, months and days because comparing two timestamps is better!
So you just have to feed the is25YearsOld function with a yyyymmdd-date and it will tell if you are older/younger than 25.

function is25YearsOld(input) {
    // the date of birth of a 25 year old
    var dob25 = new Date();
    // setFullYear to the current year (eg. 2016) and subtract 25
    dob25.setFullYear(dob25.getFullYear() - 25);
    // set the time to midnight
    dob25.setHours(0);
    dob25.setMinutes(0);
    dob25.setSeconds(0);
    // get the unix-timestamp
    dob25 = dob25.getTime();
    // the user's date of birth
    var dob = new Date();
    // set the year, month and day accordingly to the input
    dob.setFullYear(parseInt(input.substr(0, 4)));
    dob.setMonth(parseInt(input.substr(4, 2)) - 1); // (-1) because month is zero-based
    dob.setDate(parseInt(input.substr(6, 2)));
    // set the time to midnight
    dob.setHours(0);
    dob.setMinutes(0);
    dob.setSeconds(0);
    // get the unix-timestamp
    dob = dob.getTime();
    // if the timestamp of requiredDateOfBirth is smaller than the one of dateOfBirth, the user is too young
    return dob25 >= dob;
}

// Oct 10, 2008
document.getElementById("test1").innerHTML = is25YearsOld("20081010") ? "You are old enough!" : "You are too young!";
// Oct 10, 1988
document.getElementById("test2").innerHTML = is25YearsOld("19881010") ? "You are old enough!" : "You are too young!";

2008-10-10:

1988-10-10:


Multiple if-statements with overwriting lists I currently have the following piece of code in place that attempts to find matches within a list, based on a priority system:
possible_combinations = ["ABC", "AB", "A", "D"]

if len(possible_combinations) >= 1:
    chosen_combination = [comb for comb in possible_combinations if ("A" in comb) and ("C" in comb)]
    if (len(chosen_combination) == 0):
        chosen_combination = [comb for comb in possible_combinations if ("A" in comb)]
        if (len(chosen_combination) == 0):
            chosen_combination = [comb for comb in possible_combinations if ("C" in comb)]
            if (len(chosen_combination) == 0):
                raise ValueError(f"No match found.")

print(chosen_combination)

['ABC']

Is there any way I could refactor this code to get rid of these if-statements, and thereby make my code look nicer?

You could do something like this:
possible_combinations = ["ABC", "AB", "A", "D"]
combs = [("A","C"),("A",),("C",)]

for comb in combs:
    chosen_comb = [x for x in possible_combinations if all([y in x for y in comb])]
    if len(chosen_comb) > 0:
        break

print(chosen_comb)


Basic Nginx server environment creation script Here's a script I wrote for raising up a basic Nginx server environment.

The script includes an internal script named csb (create Sblock) which is used to create Sblocks in a comfortable way. As I'm new to Linux, I might have had about 2-3 mistakes, especially in that internal script.

#!/bin/bash -x
function nginx {

    apt-get update -y && apt-get upgrade -y
    apt-get install unattended-upgrades tree zip unzip -y
    dpkg-reconfigure --priority=low unattended-upgrades
    mkdir /root/backups /root/backups/db /root/backups/dirs

    ufw app list # Orient your firewall for Nginx.
    cd /usr/src && rm -fv csf.tgz
    wget https://download.configserver.com/csf.tgz && tar -xzf csf.tgz
    cd csf && sh install.sh
    sed -i 's/TESTING = "1"/TESTING = "0"/g' /etc/csf/csf.conf
    csf -r && perl /usr/local/csf/bin/csftest.pl

    cd /usr/local/src && wget http://www.rfxn.com/downloads/maldetect-current.tar.gz && tar -xzf maldetect-current.tar.gz
    cd maldetect-* && bash ./install.sh && cd ~

    # systemctl stats/stop/start/restart/reload/disable/enable # disable/enable --- at boot. # nginx -t
    apt-get install nginx mysql-server php-fpm php-mysql
    sed -i 's/;cgi.fix_pathinfo=1/cgi.fix_pathinfo=0/g' /etc/php/7.0/fpm/php.ini
    sed -i 's/server_names_hash_bucket_size ..;/server_names_hash_bucket_size ..;/g' /etc/nginx/nginx.conf
    restart php7.0-fpm
    # http://ip_address

    cat <<-'CSB' > /etc/nginx/sites-available/csb
        #!/bin/sh
        for domain; do
        > "/etc/nginx/sites-available/${domain}.conf" cat <<EOF
        server {
            listen 80;
            listen [::]:80;
            root /var/www/html/${domain};
            index index.php index.html index.htm index.nginx-debian.html;
                server_name ${domain} www.${domain};

            location / {
                try_files $uri $uri/ =404;
            }

            location ~ \.php$ {
                include snippets/fastcgi-php.conf;
                fastcgi_pass unix:/run/php/php7.0-fpm.sock;
            }

            location ~ /\.ht {
                deny all;
            }
        }
        EOF
    ln -s /etc/nginx/sites-available/${domain} /etc/nginx/sites-enabled/
        done
        systemctl restart nginx.service
    CSB
    chmod +x /etc/nginx/sites-available/csb

    cat <<-'BASHRC' > /etc/bash.bashrc
    alias csb="bash /etc/nginx/sites-available/csb"
    BASHRC

    reboot
} nginx

Note: For some reason I can't indent the code under ln, here in Code Review.

Error handling

The script misses error handling at several places.
The downloads could fail or be corrupt,
but the script will continue until the end,
and finding the failure point can be difficult.
Worse, if a failure is quiet enough,
it might not even get noticed.

Checking the exit code of every single command can be tedious.
An alternative is to add this near the top of the script:

set -euo pipefail

This will cause the script abort on the first failure,
and also treat suspicious operations as failure.

Long lines

Long lines are harder to read.
Instead of this:

cd /usr/local/src && wget http://www.rfxn.com/downloads/maldetect-current.tar.gz && tar -xzf maldetect-current.tar.gz

It would be easier to read with one line per command:

cd /usr/local/src \
  && wget http://www.rfxn.com/downloads/maldetect-current.tar.gz \
  && tar -xzf maldetect-current.tar.gz

Changing directories in scripts

Be careful with changing directories in scripts.
If an error happens in the middle of the operation,
the next operation may find itself in an unexpected directory (in the absence of adequate error handling).

Changing directories can become safer using (...) sub-shells.
For example instead of this, with a cd ~ at the end:

cd /usr/local/src && wget http://www.rfxn.com/downloads/maldetect-current.tar.gz && tar -xzf maldetect-current.tar.gz
cd maldetect-* && bash ./install.sh && cd ~

You could do this, without the need for the cd ~ at the end:

(
    cd /usr/local/src && wget http://www.rfxn.com/downloads/maldetect-current.tar.gz && tar -xzf maldetect-current.tar.gz
    cd maldetect-* && bash ./install.sh
)

Function declaration style

This is the recommended writing style:

nginx() {
    # ...
}

That is, no function keyword, and () after the name.

I would also give this function a more descriptive name.

Tips

Instead of:

mkdir /root/backups /root/backups/db /root/backups/dirs

You could write more compactly as:

mkdir -p /root/backups/{db,dirs}

Instead of cd ~, you could write simply cd.


How to optimize the for loop for finding a matching 2 string using fuzzywuzzy I am getting the probability of a string being similar to another string in Python using fuzzywuzzy lib.

Currently, I am doing this using a for loop and the search is time consuming.

Below is working code :

from fuzzywuzzy import fuzz

with open('all_nut_data.csv', newline='') as csvfile:
   spamwriter = csv.DictReader(csvfile)
   mostsimilarcs = 0
   mostsimilarns = 0
   for rowdata in spamwriter:
       mostsimilarns = fuzz.ratio(rowdata["Food Item"].lower(), name.lower())
       if mostsimilarns > mostsimilarcs:
           mostsimilarcs = mostsimilarns
           row1 = rowdata

How I can optimize this code without for loop?

Note* CSV file contain 600,000 rows and 17 column

This will not be much faster, but more readable (IMO) and extendable. You are looking for the maximum (in similarity). So, use the built-in max function. You can also define a function that does the file reading (so you can swap it out for a list of dictionaries, or whatever, for testing) and a function to be use as key. I made it slightly more complicated than needed here to give some customizability. The word it is compared to is fixed, so it is passed to the outer function, but so is the column name (you could also hard-code that).

import csv
from fuzzywuzzy.fuzz import ratio as fuzz_ratio

def get_rows(file_name):
    with open(file_name, newline='') as csvfile:
        reader = csv.DictReader(csvfile)
        yield from reader

def similarity_to(x, column_name):
    x = x.lower()
    def similarity(row):
        return fuzz_ratio(row[column_name].lower(), x)
    return similarity

if __name__ == "__main__":
    items = get_rows('all_nut_data.csv')
    name = "Hayelnut"
    best_match = max(items, key=similarity_to(name, "Food Item"))
    match_quality = similarity_to(name, "Food Item")(best_match)

max ensures that the key function is only called once per element (so no unnecessary calculations). However, since the similarity is not part of the row, you have to calculate it again at the end. On the other hand, I don't call name.lower() every loop iteration.
 Note that get_rows is a generator. This is very nice because you don't need to load the whole file into memory (just like in your code), but if you want to run it multiple times, you need to recreate the generator each time.

In the end the code as currently written can not avoid having to call the function on each row, one at a time. With max at least the iteration is partially done in C and therefore potentially faster, but not by much. For some naive tests, the built-in max is about 30% faster than a simple for loop, like you have.

The only way to get a significant speed increase would be to use a vectorized version of that function. After some digging I found out that internally the fuzzywuzzy just returns the Levenshtein ratio for the two words (after type and bound checking, and then applies some casting and rounding) from the Levenshtein module. So you could look for different modules that implemented this or try if directly using the underlying method is faster. Unfortunately I have not managed to find a vectorized version of the Levenshtein ratio (or distance) where one word is fixed and the other is not.

However, there is fuzzywuzzy.process.extractOne, which lets you customize the scoring and processing. It might be even faster than the loop run by max:

from fuzzywuzzy import process, fuzz

def processor(x):
    return x["Food Item"].lower()

def get_best_match(name, rows):
    name = {"Food Item": name}
    return process.extractOne(name, rows,
                              scorer=fuzz.ratio, processor=processor)

if __name__ == "__main__":
    rows = get_rows('all_nut_data.csv')
    name = "Hayelnut"
    best_match, match_quality = get_best_match(name, rows)
    print(best_match, match_quality)

The packing of the name in the dictionary is necessary, because the processor is called also on the query.

Using a local dictionary (from the hunspell package), which contains 65867 words, I get the following timings for finding the closest match for "Hayelnut":

OP:             207 ms ± 4.05 ms
max:            206 ms ± 8.33 ms
get_best_match: 221 ms ± 3.77 ms

So no real improvement, in fact the last function is even slightly slower! But at least all three determine that "hazelnut" is the correct choice in this case.


Project Euler #47: find four consecutive integers, each with four distinct prime factors Project Euler Problem 47

  Find the first four consecutive integers to have four distinct prime factors each. What is the first of these numbers?

This is my solution for problem 47 on Project Euler.

def primes2(n):
    """ Input n>=6, Returns a list of primes, 2 <= p < n """
    n, correction = n-n%6+6, 2-(n%6>1)
    sieve = [True] * (n//3)
    for i in range(1,int(n**0.5)//3+1):
      if sieve[i]:
        k=3*i+1|1
        sieve[      k*k//3      ::2*k] = [False] * ((n//6-k*k//6-1)//k+1)
        sieve[k*(k-2*(i&1)+4)//3::2*k] = [False] * ((n//6-k*(k-2*(i&1)+4)//6-1)//k+1)
    return [2,3] + [3*i+1|1 for i in range(1,n//3-correction) if sieve[i]]

def findSeq(lst):
    """ Returns the index of the first term of a 4 term arithmetic sequence """
    for x in range(len(lst)-2):
        if sum(lst[x:x+4]) == (4*lst[x]+6):
                return x
    return False

def generateFactors(num,values,factors):
    if len(values[num]) > 0:
        factors += values[num]
        return factors
    else:
        for i in range(2,num-1):
            if num % i == 0:
                return generateFactors(num//i,values,factors+[i])
n = 200000

primes = set(primes2(n))

factors = dict([[x,[]] for x in range(2,n)])

for item in factors.keys():
    if item in primes:
        factors[item] = [item]
    else:
        factors[item] = generateFactors(item,factors,[])

fourFact = [item for item in factors.keys() if len(set(factors[item])) == 4]

pos = findSeq(fourFact)

print(fourFact[pos])

The primes2 function isn't my code but I have included it for completeness. I am looking for feedback on my implementation and how to optimise the code.

Since you already have a nice implementation of a prime sieve, you should make use of it when checking for prime factors. Note that prime factors are needed, not all factors. I definitely would not use a recursive algorithm for this. It can fail with a stack limit and you need a lot of unreadable code just to set up the data structures.

I would use something simple like this:

from math import sqrt

def prime_factors(n, primes=None):
    """(int) -> iterable of ints

    Yields the prime factors of a number, excluding 1.
    Optionally takes an iterable of pre-computed prime numbers.
    """
    max_p = int(sqrt(n)) + 1
    if primes is None:
        primes = primes2(max_p)
    for p in primes:
        while n > 1 and n % p == 0:
            n //= p
            yield p
        if n == 1 or p >= max_p:
            break
    if n > 1:
        yield n

The second part of the task can be done in multiple ways. You have chosen to just define a maximum value up to which to search, filter all numbers with the right number of prime factors and then search for an increasing sequence. This will take quite a bit of memory, potentially, depending on where the answer is.

An easier way is to only remember the last four numbers with the right number of prime factors using a collections.deque with a maxlen=4, and check if they are an increasing sequence:

from itertools import count
from collections import deque

def diff(it):
    it = iter(it)
    x = next(it)
    for y in it:
        yield y - x
        x = y

def is_increasing_sequence(it):
    return all(d == 1 for d in diff(it))

if __name__ == "__main__":
    d = deque([], maxlen=4)
    for n in count(1):
        if len(set(prime_factors(n))) == 4:
            d.append(n)
            if len(d) == 4 and is_increasing_sequence(d):
                print(d[0])
                break

This can be sped up by re-introducing a maximum number to check and pre-computing the prime numbers for the prime number factorization:

if __name__ == "__main__":
    max_n = 200000
    primes = primes2(max_n)
    d = deque([], maxlen=4)
    for n in range(1, max_n):
        if len(set(prime_factors(n, primes=primes))) == 4:
            d.append(n)
            if len(d) == 4 and is_increasing_sequence(d):
                print(d[0])
                break

This solves the problem in less than a second on my machine, which is similar to your code.

Note that I followed Python's official style-guide, PEP8, which recommends lower_case for functions and variables and surrounded my code with an if __name__ == "__main__": guard to allow importing from this script without running the code. This is especially important for Project Euler, as you will need e.g. the prime sieve and the prime number factorization again in other problems.


Replicating the norwegian flag using CSS so here I am trying to get some reviews from my code, where I have to replicate the norwegian flag using flexbox.
First to be sayed is that my teachers told me to divide the flag in two sections, where I have had to insert two of the four squares in eachone, but I ignored this since with declarations like align-content: space-between; I could send the four squares right to the corners and play with margins to adjust them, that's why I just created 4 containers (white squares) with its respective item (red squares).
Before I show my code, I want to tell that it was so diffuicult to me to deal with the size of the elements, first a try to use flex-basis: ; and flex-grow: ; to automatically give size to the squares, but this does'nt work for me, so I have to use width: ; and height: ;.
Due to all this I want to get reccomendations to improve my code or best alternatives to reach the objective and then a kind of explanation about the size of elements.
Here's the code:

body{
    background-color: #00205B;
    margin: 0;
    display: flex;
    flex-wrap: wrap;
    align-content: space-between;
}
.white-square1, .white-square2, .white-square3, .white-square4{
    background-color: #ffffff;
    display: flex;
    align-content: flex-start;
    justify-content: space-between;
    align-items: flex-start;
}
.white-square1, .white-square3{
    margin: 0 50px 50px 0;
    height: 43.8vh;
    width: 25vw;
    flex-grow: 1;
}
.white-square2, .white-square4{
    margin: 0 0 50px 50px;
    height: 43.8vh;
    width: 50vw;
    flex-grow: 2;
}
.white-square3{
    margin: 50px 50px 0 0;
    align-items: flex-end;
}
.white-square4{
    margin: 50px 0 0 50px;
    align-items: flex-end;
}
.red-square1, .red-square2, .red-square3, .red-square4{
    background-color: #BA0C2F;
}
.red-square1, .red-square3{
    margin: 0 50px 50px 0;
    height: 37.8vh;
    width: 25vw;
    flex-grow: 1;
    margin-left: 0;
}
.red-square2, .red-square4{
    margin: 0 0 50px 50px;
    height: 37.8vh;
    width: 50vw;
    flex-grow: 2;
    margin-right: 0;
}
.red-square3{
    margin: 50px 50px 0 0;
    align-items: flex-end;
}
.red-square4{
    margin: 50px 0 0 50px;
    align-items: flex-end;
}
<!DOCTYPE html>

    <meta charset="UTF-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <link rel="stylesheet" href="style.css">
    <title>Flexbox Actividad 3</title>

Quick implementation idea
Create a container div with two divs inside. Position those divs as a cross lust like in the Norwegian flag. After that give those divs a thick white border and the flag's red color would be simply the container div's background-color.
Code review

Sections are not an appropriate HTML element in this case. Use div's instead.
Do not style using html tags like body. Use CSS classes instead. Read about the css specificity
.red-square2, .red-square4{ this selector has both margin and margin-righ properties at once. margin: a b c d should be enough.
Tidy up your CSS. In one line You style .red-square3 and some lines below You style the same element. Why? Can't you use one CSS rule for that? :)

Improvement ideas

Move the CSS color to variables and place them in a separate file.


Find an Element in a Linked List The find function is designed to start at the given node and return the index of the node with the value valueInput (indices start at 0). Return -1 if valueInput does not exist.
How can this code be improved, optimized and free of potential runtime errors?
Please note I am new to data structures and algorithms.

#include <iostream>

class Node {
    public:
        int value;
        Node* next = NULL;
};

void push(struct Node** head_ref, int new_data)
{
    /* allocate node */
    struct Node* new_node =
            (struct Node*) malloc(sizeof(struct Node));

    /* put in the data */
    new_node->value = new_data;

    /* link the old list off the new node */
    new_node->next = (*head_ref);

    /* move the head to point to the new node */
    (*head_ref) = new_node;
}

int find(struct Node *head, int n)
{
    int count = 1;

    //if count equal too n return node->data
    if(count == n)
    return head->value;

    //recursively decrease n and increase
    // head to next pointer
    return find(head->next, n-1);
}

int main()
{
    struct Node* head = NULL;

    push(&head, 1);
    push(&head, 4);
    push(&head, 1);
    push(&head, 12);
    push(&head, 1);

    printf("Element at index 3 is %d", find(head, 3));
    getchar();
    return 0;
}

Overall

You don't use encapsulation. Which makes your list vulnerable to incorrect initialization and accidental incorrect modification from outside the list.

You use several C based style choice rather than C++ style which make your codde harder to read.

Code Review

Only a list of int?

class Node {
    public:
        int value;          // int only
        Node* next = NULL;
};

Passing a pointer to a pointer. You can simplify this by passing a reference.

void push(struct Node** head_ref, int new_data)

In C++ you don't need to use struct keyword when using struct types.

void push(struct Node** head_ref, int new_data)

A better declaration would have been:

void push(Node*& head_ref, int new_data)

C++ you should always use new (rather than the malloc family).

    /* allocate node */
    struct Node* new_node =
            (struct Node*) malloc(sizeof(struct Node));

There are two reasons for this:

If your code combines both C and C++ memory allocation you need to track which is which and use the correct de-allocation method. Thus it is best to simply use one allocation method then you always know how to deallocate it.
Using new calls the constructor to initialize the object.
Remember this line from your class declaration.

    Node* next = NULL;

This is not going to happen if you call malloc() you must use new to get that to happen.

Its also simpler to write:

Node* new_node = new Node{new_data, *head_ref};

Your find returns the nth index of the list. But your index is 1 based. Most C based languages use a zero based index. But if I pass 0 to find() this function will recurse for ever.

In recursive funtions always check for the end of the recursion first. So as the first check in find you should check that the list pointer is not nullptr.

This is not modified.

    int count = 1;

So this should be a constexpt. The whole point of using a named type is to make the code more expressive. A better name would help the code be more expressive.

Don't leave redundant code commented out. Delete it.

    //if count equal too n return node->data

Source control system allow you to keep older versions of the code around

It is now easy to install git on all machines learn to use it.

Use better indentation

    if(count == n)
    return head->value;

In C++ we use nullptr rather than NULL.

    struct Node* head = NULL;

The difference is that nullptr is correctly typed as a pointer, while NULL is a macro (bad) for an integer (bad type). Thus you can not incorrectly use nullptr while NULL can be abused.

In C++ we use the C++ streams std::cout.

    printf("Element at index 3 is %d", find(head, 3));

The C++ streams have a more advanced type checking system that prevents accidents.

    std::cout << "Element at index 3 is " << find(head, 3);

Beter implementation

template<typename T>
class LinkedList
{
    struct Node {
            T value;
            Node* next;
    };
    Node*   root;

    public:
        LinkedList()
            : root(nullptr)
        {}
        ~LinkedList() {
            while(root) {
                Node* next = root->next;
                delete root;
                root = next;
            }
        }
        LinkedList(LinkedList const&)            = delete;
        LinkedList& operator=(LinkedList const&) = delete;

        void push(T const& new_data)
        {
            root= new Node{new_data, root};
        }

        int find(int n)
        {
            Node* result = findElement(root, n);
            if (result == nullptr) {
                throw std::runtime_error("message");
            }
            return result->value;
        }
    private:
        Node* findElement(Node* n, int n) {
            if (n == nullptr) {
                return nullptr;
            }

            if (n == 0) {
                return n;
            }
            return findElement(n->next, n-1);
        }
}

Main.cpp

int main()
{
    LinkedList<int> list;

    list.push(1);
    list.push(4);
    list.push(1);
    list.push(12);
    list.push(1);

    std::cout << "Element at index 3 is " << find(head, 2) << "\n";
    getchar();
}


Cleaning up validator class I updated my class with more functionality, but I kind if feel like it's a mess or that it can at least be improved by a bit readability, efficiency and/or DRY wise.

Anybody who could see if that can be done?

Any other suggestions are also welcome.

class Validator
{
    private $errors = [];
    private $rules  = [];

    public function getErrors($rulename = null)
    {
        if ($rulename) {
            return isset($this->errors[$rulename]) ? $this->errors[$rulename] : null;
        }

        return $this->errors;
    }

    # rules will be checked against in order as specified
    public function setRule($name, Array $rules, $required = true)
    {
        if (!array_key_exists($name, $this->rules)) {
            foreach ($rules as $rule) {
                if (!$rule instanceof IRule) {
                    throw new Exception('Array must only contain instances of IRule.');
                }
            }

            $this->rules[$name]['rules'] = $rules;

            if ($required === true) {
                $this->rules[$name]['required'] = 'Must not be null or empty.';
            } elseif ($required === false) {
                $this->rules[$name]['required'] = false;
            } else {
                $this->rules[$name]['required'] = (String) $required;
            }
        } else {
            throw new Exception(sprintf('Rule %s already exists.', $name));
        }
    }

    public function validate($rulename, $input)
    {
        if ($this->rules[$rulename]['required']) {
            if (empty($input)) {
                $this->errors[$rulename][] = $this->rules[$rulename]['required'];
            }

            foreach ($this->rules[$rulename]['rules'] as $rule) {
                try {
                    $rule->check($input);
                } catch (Exception $e) {
                    $this->errors[$rulename][] = $e->getMessage();
                }
            }

            if (isset($this->errors[$rulename])) {
                return false;
            }
        } elseif (!empty($input)) {
            foreach ($this->rules[$rulename]['rules'] as $rule) {
                try {
                    $rule->check($input);
                } catch (Exception $e) {
                    $this->errors[$rulename][] = $e->getMessage();
                }
            }

            if (isset($this->errors[$rulename])) {
                return false;
            }
        }

        return true;
    }

    public function dump()
    {
        print_r($this->rules);
    }
}

Good work formatting and approaching styles in a fashionable method, it makes reading your code very easy! a couple things to note:

It's common to see private variables prefixed with an underscore to improve readability. It's up to you if you want to implement this, and if you use a framework, check their docs to see if it's standard.
Regarding your last method, dump(): the name dump could mean many things. Without seeing the source, what's it dumping? It'd be fine to leave it as is, but you may consider renaming to dumpRules() or something of that sort.
I'm not sure if you left it out, or you just haven't, but it's also a good practice to document your code. That means add comments. Googling PHPDoc will give you a good start, and if your familiar with JavaDocs, that's what I'm talking about.

Be careful making new array's with just = [].

  As of PHP 5.4 you can also use the short array syntax, which replaces
  array() with [].

That's from the docs. So if anyone using below 5.4 wants your code, they could experience some turbulence setting it up.

The one comment you do have, is started with #. I believe this style of comments are deprecated and the C style comments (/*...*/, //) are favorable. I'll try and find a reference to this claim.

I noticed you have

foreach ($this->rules[$rulename]['rules'] as $rule) {
    try {
        $rule->check($input);
    } catch (Exception $e) {
        $this->errors[$rulename][] = $e->getMessage();
    }
}

a couple times. You could put this in a function (with appropriate parameters) and simply call the function to reduce multiplicities.

Other than that, all looks well. Just in case you wanted to look at some other validation classes (if you haven't already), here's one to get you started.


Multi-threaded in-place Mergesort on Java What is an efficient way to implement the Mergesort algorithm in Java such that it meets the following criteria:

Must be multi-threaded.
Must retain the Mergesort time complexities.
Must be in-place implementation, no copying arrays.
Must use Java Collection classes, not an array.
Must work on any Java Comparable.

My solution involves using an ArrayList and fork-join to create the in-place Mergesort. I've tested this on multiple inputs and they all work.

I'm curious if anyone can find a way I can improve my code. Either a more efficient solution or perhaps an edge case I didn't address.

import java.util.*;
import java.util.concurrent.*;

public class MergeSort<N extends Comparable<N>> extends RecursiveTask<List<N>> {
    private List<N> elements;

    public MergeSort(List<N> elements) {
        this.elements = elements;
    }

    @Override
    protected List<N> compute() {
        if(this.elements.size() <= 1)
            return this.elements;
        else {
            final int pivot = this.elements.size() / 2;
            MergeSort<N> leftTask = new MergeSort<N>(this.elements.subList(0, pivot));
            MergeSort<N> rightTask = new MergeSort<N>(this.elements.subList(pivot, this.elements.size()));

            leftTask.fork();
            rightTask.fork();

            List<N> left = leftTask.join();
            List<N> right = rightTask.join();

            merge(left, right);
            return this.elements;
        }
    }

    private void merge(List<N> left, List<N> right) {
        int leftIndex = 0;
        int rightIndex = 0;
        while(leftIndex < left.size()) {
            if(rightIndex == 0) {
                if( left.get(leftIndex).compareTo(right.get(rightIndex)) > 0 ) {
                    swap(left, leftIndex++, right, rightIndex++);
                } else {
                    leftIndex++;
                }
            } else {
                if( right.get(0).compareTo(right.get(rightIndex)) < 0 ) {
                    swap(left, leftIndex++, right, 0);
                } else {
                    swap(left, leftIndex++, right, rightIndex++);
                }
            }
        }

        if(rightIndex < right.size() && rightIndex != 0)
            merge(right.subList(0, rightIndex), right.subList(rightIndex, right.size()));
    }

    private void swap(List<N> left, int leftIndex, List<N> right, int rightIndex) {
        //N leftElement = left.get(leftIndex);
        left.set(leftIndex, right.set(rightIndex, left.get(leftIndex)));
    }

    public static void main(String[] args) {
        ForkJoinPool forkJoinPool = ForkJoinPool.commonPool();
        List<Integer> result = forkJoinPool.invoke(new MergeSort<Integer>(new ArrayList<>(Arrays.asList(1,3,5,7,2,4,6))));
        System.out.println("result: " + result);
    }
}

Update: The code will fail on the edge case where the following input integers are used:

{5,9,8,7,6,1,2,3,4}

This occurs because the recursive call to merge only works successfully when the left and right lists are of equal length or differ by only one in length. This call however will result in the comparison of two lists that breaks that expectation. I have yet to find a solution which handles all edge cases properly.

Quick-Glance nitpicks

elements can (and should) be final. This reinforces the point that you're not changing that reference.
Always place braces around blocks. That's especially true for single-line if-blocks like the element size guard.
Somewhat interestingly inside merge you placed the braces in all if-blocks except the last.
You're inconsistent in the spacing for conditionals. In merge the inner if-statement's conditionals are surrounded by spaces, everywhere else, they are not. I personally prefer putting a space before the opening parenthesis of any condition, if only to distinguish it from a method invocation.

Density of semantics

The swap calls in merge are rather heavy in what they do. There's a lot going on at once, relying on a conscious and full understanding of unary operators and how the method works.

If I were you, I'd consider extracting the incrementing of the indices to separate statements. That can also make the reasoning somewhat easier to understand, seeing that leftIndex++ is in every branch of the tree:

    while (leftIndex < left.size()) {
        if (rightIndex == 0) {
            if (left.get(leftIndex).compareTo(right.get(rightIndex)) > 0) {
                swap(left, leftIndex, right, rightIndex);
                rightIndex++;
            }
        } else if (right.get(0).compareTo(right.get(rightIndex)) > 0) {
            swap(left, leftIndex, right, 0);
        } else {
            swap(left, leftIndex, right, rightIndex);
            rightIndex++;
        }
        leftIndex++;
    }

At this point it should also be clear that the first if-statements can be collapsed into a single statement.
  JS1 mentions that the in-place merge you are doing does not work for all cases.
  Unfortunately I can't tell you quite exactly why, but making the code easier to grasp should help troubleshooting and fixing this :)

Optional List features

You're making use of the set operation on Lists.
It's important to be aware that set is an optional operation.
Not all implementations of List provide an implementation and may throw UnsupportedOperationExcpetion instead.
Then again all the other features you could use to replace this are also optional (what with modification of Lists being optional)...


javascript - consolidate multiple null/undefined checks My brain is fried from looking from looking at this for so long.  Is there a more efficient way to do this code block.  We have the ability to use es6 code too if that helps.

First it tries to set quantity to itemTemplateIDQty.value if itemTemplateQty is not valid and itemTemplateIDQty is valid.  Then it basically checks the opposite and sets quantity to itemTemplateQty.value.  I'm thinking since in that else block, I am simply checking the exact opposite, I shouldn't need to retype all that code again.  Also I'm wondering if I can remove a lot of code by using something like quantity = itemTemplateIDQty.value || 1;.

Update
I'll try to sum up the code more clearly.  The default value for quantity should be 1 if all the checks fail.  First if there are no classes of itemTemplateQty found and 1+ classes of 'itemTemplateQty' + itemId found then set quantity to the value of the first class found of 'itemTemplateQty' + itemId.  If that check fails, then check if no classes of 'itemTemplateQty' + itemId are found and 1+ classes of itemTemplateQty are found, set quantity to the value of the first class of itemTemplateQty.

let itemId = 5;
let isDynamicRecommendation = false;
let itemTemplateQty =
        document.getElementsByClassName('itemTemplateQty').length > 0 ? document.getElementsByClassName('itemTemplateQty') : null;
    let itemTemplateIDQty =
        document.getElementsByClassName('itemTemplateQty' + itemId).length > 0 ? document.getElementsByClassName('itemTemplateQty' + itemId) : null;
    let quantity = 1;
    if (!isDynamicRecommendation) {
      if ((itemTemplateQty === null || itemTemplateQty.value == null || Number.parseInt(itemTemplateQty.value) <= 0) &&
         (itemTemplateIDQty !== null && itemTemplateIDQty.value !== undefined && itemTemplateIDQty.value !== null && Number.parseInt(itemTemplateIDQty.value) > 0)) {
           quantity = itemTemplateIDQty.value;
      } else if ((itemTemplateIDQty === null || itemTemplateIDQty.value == null || Number.parseInt(itemTemplateIDQty.value) <= 0) &&
        (itemTemplateQty !== null && itemTemplateQty.value !== undefined && itemTemplateQty.value !== null && Number.parseInt(itemTemplateQty.value) > 0)) {
          quantity = itemTemplateQty.value;
      }
    }
console.log("quantity: ", quantity);

.getElementsByClassName() returns an HTMLCollection, not a single element. You can use bracket notation [n] to get an element at index n of the collection returned.

One if statement can be used to evalaute !itemData.isDynamicRecommendation. One if..else..if can be used to evaluate itemTemplateQty[0] && itemTemplateQty[0].value <= 0 or itemTemplateIDQty[0] && itemTemplateIDQty[0].value <= 0.

To convert the string .value to an integer you can use + operator preceding +itemTemplateQty[0].value.value or alternatively use .valueAsNumber.

let itemData = 5;
let isDynamicRecommendation = false;

let itemTemplateQty = document.getElementsByClassName('itemTemplateQty')[0];
let itemTemplateIDQty = document.getElementsByClassName('itemTemplateQty' + itemData)[0];
let quantity = 1;
if (!isDynamicRecommendation) {
  if (itemTemplateQty && itemTemplateQty.value <= 0) {
    quantity = +itemTemplateQty.value;
  } else if (itemTemplateIDQty && itemTemplateIDQty.value <= 0) {
    quantity = +itemTemplateIDQty.value;
  }
}

console.log({quantity});


Application container design I'm putting together a simple application container for learning purposes, and I would like to hear how you see this code. The application is the main class that will contain services, configuration and other related stuff.

I have identified 3 position where I can't decide on the design:

Each application should be supplied with configuration array. The configuration will be loaded automatically, so some class has to deal with it. Does Kernel class is the right place to put it? The Kernel class is intended to contain some common functions, like getRootPath(). Does it starts to violate SRP in this case?
Does getType() look like the right way to define type of application? Should i make it abstract or provide some default type in parent class?
Overall, does Kernel class fits the purpose of creating application instances and supplying required configuration and calling ready() on application?

namespace App;

interface Executable {
    public function run();
    public function ready();
}

abstract class Application implements Executable {
    const TYPE_WEB = 'web';
    const TYPE_CLI = 'cli';

    /**
    * Application name
    * @var string
    */
    protected $name = '';

    /**
    * Application configuration
    * @var array
    */
    protected $config = [];

    public function __construct($name, array $config=[]) {
        $this -> name = $name;
        $this -> config = $config;
    }

    public function ready() {
        // Application is ready, register services here...
    }

    public function run() {
        // Execute application lifecycle
    }
}

class WebApplication extends Application {
    public function getType() {
        return self::TYPE_WEB;
    }
}

class ConsoleApplication extends Application {
    public function getType() {
        return self::TYPE_CLI;
    }
}

abstract class Kernel {
    /**
    * Instantiated applications
    * @var Executable[]
    */
    private static $apps = [];

    /**
    * Creates new application
    * @param string $name Application name
    * @param string $type Application class
    */
    public static function createApp($name, $type) {
        /* Some validations skipped */

        $app = self::$apps[$name] = new $type($name, self::loadConfiguration($name));
        $app -> ready();

        return $app;
    }

    private static function loadConfiguration($name) {
        /* Load configuration from files and from multiple places */
        return [
            'key' => 'value'
        ];
    }

    public static function getRootPath() {
        // Return path to document root
    }
}

/**************/
App\Kernel::createApp('test', 'WebApplication') -> run();

Some opinionated advice:

I don't see why having two separate class, so I merge Kernel into App and rename it "AppKernel".

Then, I removed the "ready" method to replace it by the "create" from Kernel.
You don't need the "getType" function since your classes already define a "type"
I don't really understand why you want to keep a reference to your application's instances, so I remove it.
For me you should load the configuration from the outside with a dedicated "configuration" object.

EDIT: I added a method to create "singleton" application, I think this method will work but I am not 100% sure, so try it before.

All this transformation lead to something like this:

<?php
namespace App;

interface Executable
{
    public function run();
    public function ready();
}

class Configuration
{
    public static function load() {
        return [
            'key' => 'value'
        ];
    }
}

abstract class AppKernel implements Executable
{
    /**
     * Application name
     * @var string
     */
    protected $name = '';
    private static $instances = [];
    /**
     * Application configuration
     * @var array
     */
    protected $config = [];

    protected function __construct($name, Configuration $config)
    {
        $this->name = $name;
        $this->config = $config;
    }

    public static function create($name, Configuration $config)
    {
        if(!isset(static::$instance[$name]))
           static::$instance[$name] = new static($name, $config);

        return static::$instance[$name];
    }

    public function run()
    {

    }
}

class WebApplication extends AppKernel {}

class ConsoleApplication extends AppKernel {}

ConsoleApplication::create('name', configuration::load())->run();


Simple stringifying using << operator Background

I wrote my_length class and some code. my_length can be stringified using << operator, and main() tests this.

Questions

I think operator<< can be refactored. How can I rewrite operator<< of my_length, and make it better code?
How can I make rest of code better?

Code

#include <vector>
#include <string>
#include <cstdint>
#include <iostream>

class my_length {
  int id;
  std::string name;
  uint64_t v;     // unit is 'mm'
  bool fmt_auto;

  public:
  my_length (int id, std::string name, uint64_t v)
    : id{id}, name{name}, v{v}, fmt_auto(false) {}
  friend std::ostream &operator<<(std::ostream &out, const my_length &l) {
    out << l.id << "/" << l.name << " : ";
    if (!l.fmt_auto) {
        return out << l.v << "mm";
    } else {
        if (l.v<10)
            return out << l.v << "mm";
        else if (l.v<1000)
            return out << double(l.v)/10 << "cm";
        else if (l.v<1000000)
            return out << double(l.v)/1000 << "m";
        else
            return out << double(l.v)/1000000 << "km";
    }
  }
  void set_fmt_auto(bool b) {fmt_auto = b;}
};

bool need_fmt_auto(int x) {
  return (x%2) != 0;
}

int main() {
  std::vector<my_length> lengths;
  my_length l{0, "a", 1};
  lengths.push_back(l);
  uint64_t val = 17;
  for (int i=1; i<10; i++) {
    my_length l{i, std::string(1, 'a'+i), val};
    lengths.push_back(l);
    val *= 10;
  }
  for (int i=0; i<lengths.size(); i++) {
    if (need_fmt_auto(i)) {
        lengths[i].set_fmt_auto(true);
        std::cout << "fmt_auto: ";
    }
    std::cout << lengths[i] << "\n";
  }
}

I see a number of things that may help you improve your program.

Gather constants together

The relationship among the numerical values that determine which unit is used and the divisor and the actual unit name is contained within a single function, which is good, but it could be more clear if expressed as a structure.  For example, one could use this:

struct Units {
  int limit;
  double divisor;
  std::string name;
};
static const std::array<Units,4> units;

Then outside of the class, we supply the values:

const std::array<my_length::Units,4> my_length::units = {{
  { 10, 1, "mm" },
  { 1000, 10, "cm" },
  { 1000000, 1000, "m" },
  { 0, 1000000, "km" },
}};

This brings things into much closer proximity so that it can be more easily seen and understood.  We can do a little better with C++17 by using std::string_view instead of std::string for the unit name.

Isolate concerns

It's usually better to have each function just do one thing.  So insted of deciding on units and also printing, I'd suggest splitting those into two, using a private member function:

const Units& selectUnit() const {
  auto ret{units.cbegin()};
  if (fmt_auto) {
      while (ret->limit && v >= ret->limit) {
          ++ret;
      }
  }
  return *ret;
}

Now the operator<< looks like this:

friend std::ostream &operator<<(std::ostream &out, const my_length &l) {
  const Units u{l.selectUnit()};
  return out << l.id << "/" << l.name << " : "
      << l.v/u.divisor << u.name;
}

Use standard algorithms

The test code starts with this:

std::vector<my_length> lengths;
my_length l{0, "a", 1};
lengths.push_back(l);
uint64_t val = 17;
for (int i=1; i<10; i++) {
    my_length l{i, std::string(1, 'a'+i), val};
    lengths.push_back(l);
    val *= 10;
}

I'd suggest that one could use std::iota instead.  I'll leave it to you to create the details of that.

Reconsider the design

Once constructed, there is no capability to do anything with the my_length object except to print it.  If all that's needed is printing, then it may be better to use a freestanding function in a namespace instead of an object, as suggested in the comments.  If not, then it makes sense to provide those other operations within the class.


`std::vector` 'substring concatenator' The point of this is to concatenate a vector of std::strings into one string starting at an index of the vector.

std::string vectorSubstr(std::vector<std::string> vec, int startPos, char seperator)
{
    std::stringstream ss;

    for (int i = startPos; i < vec.size(); i++)
    {
        if (i == startPos)
        {
            ss << vec[i];
        }
        else
        {
            ss << seperator << vec[i];
        }
    }

    return ss.str();
}

Is there anything wrong with this approach? Should  I be using a stringstream or just concatenating strings?

If you are just trying to concatenate vector<string> elements, why not use the following approach:

std::string seperator = " ";
std::string vectorElementHolder;

for (const auto& word : vec){

    vectorElementHolder += word + seperator;

}

Simple and effective, assuming you use C++11.
If you want to change the index where you start, you could always declare an iterator as such vector<string>::iterator to point to the index and go from there.


Transform a javascript object to an array based on value of each key in the original object I have an object of ingredients to be added to a burger. Its structure is as follows:

Input

    ingredients: {
        salad: 1,
        cheese: 2,
        meat: 2,
    }

I want to transform it to an array of JSX elements. Each key in the original object has to be mapped to the number of JSX elements equivalent to the value of the ingredient in the ingredients objects. So the result array should look like so:

Output

[
    <BurgerIngredient type="salad" />,
    <BurgerIngredient type="cheese" />,
    <BurgerIngredient type="cheese" />,
    <BurgerIngredient type="meat" />,
    <BurgerIngredient type="meat" />,
]

My solution is as follows:

function getBurgerIngredients() {
    const ingredientNames = Object.keys(props.ingredients);
    const transformedIngredients = ingredientNames.map( (ingredientName) => {
        let burgerIngredientElements = [];
        for(let i=0; i<props.ingredients[ingredientName]; i++) {
            burgerIngredientElements.push(
                <BurgerIngredient type={ingredientName} />
            )
        }
        return [...burgerIngredientElements]
    });

    return transformedIngredients;
}

I'm aware that it's not the most elegant solution. Can anyone suggest any improvement(s)?

const ingredientNames = Object.keys(props.ingredients);

Just be aware that anything that iterates/enumerates an object's keys (Object.keys, for-in, etc.) does not guarantee order. So it might not always be "salad, cheese, cheese, meat, meat" in the end. It just happens that most browsers iterate in the order they were created/added to the object, which is why it appears like so. If you want order, use an array to begin with.

    let burgerIngredientElements = [];
    for(let i=0; i<props.ingredients[ingredientName]; i++) {
        burgerIngredientElements.push(
            <BurgerIngredient type={ingredientName} />
        )
    }
    return [...burgerIngredientElements]

Also, I don't think this code is doing what you think it's doing. You're pushing to an existing array, then spreading it's contents into another array. You're essentially just shallow-cloning. It might look rendered correctly (probably because JSX now does fragments) but the structure is questionable.

Anyways, a more functional way to iterate from 0 to N is to create an array of N items (it doesn't matter what the values are) and operate on that array using array methods. One way this can be achieved is using Array and array.fill.

Also, instead of a loop and an outer variable holding an array, you can use array.reduce with the accumulator being that array. That way, you don't have that extra mutable variable.

Your code could look like this:

function getBurgerIngredients() {
  const ingredients = props.ingredients

  return Object.keys(ingredients).reduce((c, ingredientName) => {
    const range = Array(ingredients[ingredientName]).fill()

    return [...c, ...range.map(() => (<BurgerIngredient type={ingredientName} />))]
  }, []);
}


Named scopes for instances I have following relationship schema:

class Participation
  belongs_to :toon
  belongs_to :person

  validates :role, :presence => true
end

class Toon
  has_many :participations
  has_many :people, :through => :participations
end

class Person
  has_many :participations
  has_many :toons, :through => :participations
end

I want to get people association related to toon, but filtered by role attribute value. I need something like toon.people_with_role('producer').

class Toon < ActiveRecord::Base
  has_many :participations
  has_many :people, :through => :participations

  def people_with_role(role)
    Participation.where(:toon_id => self, :role => role).map(&:person)
  end
end

It works, but I'm not sure that it is a best way to do it.

Here is another way to filter people by role:

class Participation < ActiveRecord::Base
  belongs_to :toon
  belongs_to :person

  validates :role, :presence => true
end

class Person < ActiveRecord::Base
  has_many :participations
  has_many :toons, :through => :participations
end

class Toon < ActiveRecord::Base
  has_many :participations
  has_many :people, :through => :participations do
    def with_role(role)
      where('participations.role' => role)
    end
  end
end

Now, you can get the people fitered by role: toon.people.with_role('producer').


Self-made string class Since I've been working with C strings these days, I decided to make a string class from the ground-up just to see how it would go. I wrote all labels in Spanish (my mother language) just for the sake of it. If you have problems understanding it, I'll simply translate them into English

(DISCLOSURE: I'm not thinking about actually using this in production code. I was just trying something new. I think the native std::string class is much better for these things. But if you can tell me how this would perform in production code, it would be of great help!).

My main goal was to made a class that could be edited and used with legacy code that needs a const char* instead of std::string.

What do you think about it? I have some doubts about my implementation: for instance, I don't know if returning the buffer as const char* is safe, or if using static_cast is necessary.

Here is my header file (Cadena.h):

#pragma once
#include <iostream>

namespace David {

    class Cadena {
    private:
        char* bufer;
        size_t tamanio;

        size_t obtenerLongitud(const char* cadena) {
            size_t longitud = 0;
            for (longitud = 0; cadena[longitud] != '\0'; ++longitud);
            ++longitud;
            return longitud;
        }

        void borrarBufer() {
            delete[] bufer;
            bufer = nullptr;
        }

        void ajustar(size_t longitud) {
            char* cadenaTemporal = new char[longitud + 1];
            if (bufer != nullptr) {
                for (size_t i = 0; i < tamanio; ++i) {
                    cadenaTemporal[i] = bufer[i];
                }
            }
            borrarBufer();
            bufer = cadenaTemporal;
            cadenaTemporal = nullptr;
            tamanio = longitud + 1;
        }

    public:
        Cadena(const char* cadena) {
            tamanio = obtenerLongitud(cadena);
            bufer = new char[tamanio];
            for (size_t longitud = 0; cadena[longitud] != '\0'; ++longitud) {
                bufer[longitud] = cadena[longitud];
            }
            bufer[tamanio - 1] = '\0';
        }

        Cadena() {
            tamanio = 0;
            bufer = nullptr;
        }

        ~Cadena() {
            borrarBufer();
        }

        const char* formaConstante() const {
            return static_cast<const char*>(bufer);
        }

        const size_t len() const {
            return tamanio - 1;
        }

        char& operator [] (size_t indice) {
            if (indice < tamanio)
                return bufer[indice];
            return bufer[tamanio - 1];
        }

        void operator = (const char* cadena) {
            borrarBufer();
            tamanio = obtenerLongitud(cadena);
            bufer = new char[tamanio];
            for (size_t longitud = 0; cadena[longitud] != '\0'; ++longitud) {
                bufer[longitud] = cadena[longitud];
            }
            bufer[tamanio - 1] = '\0';
        }

        friend std::ostream& operator << (std::ostream& salida, const Cadena& cadena) {
            for (size_t longitud = 0; cadena.formaConstante()[longitud] != '\0'; ++longitud) {
                salida << cadena.formaConstante()[longitud];
            }
            return salida;
        }

        friend void operator >> (std::istream& entrada, Cadena& cadena) {
            if(cadena.bufer != nullptr) cadena.borrarBufer();
            char letra = ' ';
            size_t longitud = 0;
            while (letra != '\n') {
                entrada >> std::noskipws >> letra;
                cadena.ajustar(longitud);
                cadena.bufer[longitud] = letra;
                ++longitud;
            }
            cadena.bufer[longitud - 1] = '\0';
        }
    };

}

Possible undefined behaviour

There are several places that will lead to undefined behaviour.

In ajustar it can be possible that longitud < taminio. We will then happily copy into cadenaTemporal[i] where longitud < i < taminio. It's easy to create an example for this:

David::Cadena example("oh oh");
std::cin >> example;

So instead of taminio use both longitud as well as taminio as upper limit.

In operator [] (size_t indice), you return bufer[-1] on example[0] if the string hasn't been initialized. This should either get documented or checked.

Consistency

borrarBufer() should also set tamino to zero. That seems to be your "empty string" scenario.

Don't include the \0 in len()

If the string is empty, len() should return 0, not -1. strlen() or std::string::size() don't return the \0 either.

    const size_t len() const {
        return tamanio;
    }

Use common interface

operator= should return a Cadena&, not void. Similar, operator>> should return istream&, not void.

Follow the rule of five

You provide a custom constructor. Therefore, you should also provide

a custom copy constructor,
a custom move constructor,
a custom copy assignment,
a custom move assignment and
a custom destructor.

You don't provide any of them, but you should. For example, I cannot copy a Cadena at the moment. I have to write

Cadena my_copy(other.formaConstante())

Use copy and swap idiom for operator=

As soon as you have a copy constructor, you can write operator= very easy:

    void swap(Cadena & other) {
        std::swap(tamino, other.tamino);
        std::swap(bufer,  other.bufer);
    }

    Cadena& operator=(const char* cadena) {
        Cadena temporary(cadena);
        swap(cadena);
        return *this;
    }

Provide push(char) to add single characters

This will make operator>> a lot simpler.

Use a capacity

Your operator>> is \$\mathcal O(n^2)\$ since you have to realloc for every single char. Instead, allocate more memory than you actually need. Or use std::vector for your internal memory instead, it will take care of that automatically.


Nutrient and Recommendation models In Rails 3, I have two models: Nutrient and Recommendation:

Nutrient has_many Recommendation
Recommendation belongs_to Nutrient

I am trying to implement the create method of the recommendation in the controller. Is this the proper way to do it?

def create
  nutrient_id = params[:recommendation][:nutrient_id]
  if nutrient_id.blank?
    #nutrient_id was blank in the submit, get other recommendation params and re-render 'new'
    params[:recommendation].delete(:nutrient_id)
    @recommendation=Recommendation.new(params[:recommendation])
    render 'new'
  else
    @nutrient = Nutrient.find(nutrient_id)
    if @nutrient
      #nutrient was found by id, create recommendation
      @recommendation = @nutrient.recommendations.build(params[:recommendation])
      if @recommendation.save
        redirect_to @recommendation
      else
        render 'new'
      end
    else
      #nutrient was not found by id, get other recommendation params and re-render 'new'
      params[:recommendation].delete(:nutrient_id)
      @recommendation = Recommendation.new(params[:recommendation])
      render 'new'
    end
  end
end

Your approach is fundamentally flawed, I'm afraid. You're not using Rails' abilities to your advantage at all.

For a simple create operation, your controller method rarely needs to be more than 10 lines or so. There's way too much going on in yours.

Start by nesting your routes:

 resources :nutrients do
   resources :recommendations
 end

Run rake routes to see how the routes now look. Basically, a recommendation will have a URL like /nutrient/:nutrient_id/recommendation/:id.

This means you'll have to make some changes to links in your views and your redirections. Where you could previously write recommendations_path and so on you'll now have to write nutrient_recommendations_path(@nutrient) and so on. Again, rake routes will show you what helpers you now have.

In your recommendation form, you'll have to do something like:

 <%= form_for [@nutrient, @recommendation] %>

so the form's action URL will be correct. An get rid of the hidden input that I assume you've added to the form as recommendation[nutrient_id].

Inside the RecommendationsController, you can always get the "parent" nutrient by saying

 @nutrient = Nutrient.find(params[:nutrient_id])

as that param is given in the URL.

In the end, your create method will look like

def create
  @nutrient = Nutrient.find(params[:nutrient_id])
  @recommendation = @nutrient.recommendations.new(params[:recommendation])

  if @recommendation.save
    redirect_to @recommendation, :notice => "Recommendation created."
  else
    render 'new'
  end
end

See something like this post for a more thorough diff-like walkthrough of the changes to make.

Note, that if Recommendation has further nested resources, you should use "shallow" routes, but that's left as an exercise to the reader.


Marionette.js Mediator ('global' commands vs app controller) I am currently migrating a large backbone application over to Marionette and am curious as to the opinions on what is a better design for mediating activity between modules.

I have an Application that have several Modules where an action in one module might cause activity to happen in other modules.

Since these all are coupled to the Application I could simply use the Wreqr interface where:

Module 1:

someActivity: ->
   App.vent.trigger("skyFalling")

Module 2:

initialize: ->
   App.vent.on("skyFalling", @closeOrSomething)

Module 3:

initialize: ->
   App.vent.on("skyFalling", @blinkRapidly)

Or would it be better to trigger an event in Module1 where the Application (or a controller tied to the App) listens and then calls the 'closeOrSomething' and the 'blinkRapidly' functions directly on the modules (since the App is aware of all the modules that it is loading).

Application:

skyFallingEventHandler: ->
   Module2.closeOrSomething()
   Module3.blinkRapidly()

I have the exact same quandary (and found this question googling for answers). On first read through my opinion was that triggering events on the modules themselves was the best approach on the basis that this loosens any coupling between the modules and other components. However, having given it more thought it seems that this is exactly what the Wreqr interface has been designed for, so actually my approach would be to use the aggregator to handle events. I’d be interested to know which approach you decide to take.


JavaScript: Text Processor I been trying to implement a text processor in JavaScript that can handle 4 different types of operations: append, backspace, undo, redo.
The input for this text processor is an array of arraries of a single element or tuples. For example:
  const input = [['APPEND', 'Hey'], ['APPEND', ' there'], ['APPEND', '!']]
  textProcessor.process(input)
  textProcessor.text // Hey there!

  const input = [['APPEND', 'Hey'], ['APPEND', ' there'], ['APPEND', '!'], ['UNDO'], ['UNDO']]
  textProcessor.process(input)
  textProcessor.text // Hey

  const input = [['APPEND', 'Hey'], ['APPEND', ' there'], ['APPEND', '!'], ['UNDO'], ['UNDO'], ['REDO'], ['REDO']]
  textProcessor.process(input)
  textProcessor.text // Hey there!

  const input = [['APPEND', 'Hey'], ['APPEND', ' there'], ['APPEND', '!'], ['BACKSPACE']]
  textProcessor.process(input)
  textProcessor.text // Hey there

Here is my implementation
class TextProcessor {
    constructor() {
      this.undos = []
      this.redos = []
      this.text = ''
      this.operations = {
        APPEND: (text) => {
          this.undos.push(this.text)
          this.redos.length = 0
          return this.text + text
        },
        BACKSPACE: () => {
          this.undos.push(this.text)
          this.redos.length = 0
          return this.text.slice(0, -1)
        },
        UNDO: () => {
          const undo = this.undos.pop() ?? ''
          this.redos.push(this.text)
          return undo
        },
        REDO: () => {
          this.undos.push(this.text)
          return this.redos.pop() ?? this.text
        },
      }
    }

    process(input) {
      input.forEach(([operation, text]) => {
        this.text = this.operations[operation](text) ?? this.text
      })
    }
  }

I used an object to map the various operation names to their corresponding functions. Not sure if this is better than switch-case statements.

Unusual undo redo
Your undo and redo operations have a strange behavior in that they are them selves undo-able. Generally undo and redo operations are themselves not undo-able.
Bug?
Unknown commands will throw the error TypeError: this.operations[operation] is not a function
SEMICOLONS!
I think I mentioned semicolons last time I reviewed your code. If you do not wish to use them you should be familiar with ASI
Identifiers
Try to avoid string identifiers as they are memory hungry ad can be much slower depending on the number and similarity of identifiers.
One method of creating unique identifiers is via an enumeration object.
Example
const operators = {
    append: 0,
    undo: 1,
    redo: 2,
    backspace: 3,
}

The rewrite uses a function Enum that returns an object containing named integer Identifiers.
Encapsulation
Protect the state of the object. All properties of an object exposed to other code can not be trusted. Without a trusted state you can not guarantee that your code will run as expected.
A well designed encapsulated object (once tested) can NOT fail.
JavaScript has an excellent OO encapsulation model.

Closure is used to encapsulate state within the Object that do not require additional language tokens to access (eg this)
Object functions

Object.freeze will set all properties to writable: false, and the object to immutable.
Object.seal properties remain writable but the object is set to immutable.

Using closure to hold the objects state, you then create an interface to provide an interface to the state. The interface is just an object (frozen or sealed) that uses getters, setters, and functions to access state and control behavior
Be efficient
Always write code to be as efficient as possible.
Yes this is a balance between productivity and code efficiency but with a constant eye on efficiency  you become more adept at writing efficient code
Inefficient undo buffer
Your undo is expensive as it stores the complete text for every action that can be undone. You need only store each operation and associated data in the undo buffer.
This will make your code more complex but for large documents it will run much quicker and use far less memory. Clients don't see or care how things are done, they only care about what it does, if that include slow response and power hungry it is never a plus.
Rewrite
The rewrite adds a little functionality to help demonstrate some of the points above regarding the interface getters and setters.
Usage remains the same
Behavior is a little different

Undo and redo are not undo-able commands

It uses an named integer values to identify operations. Which is a static property of TextProcessor.operators and internally accessed via operators

The function TextProcessor.process ensures input is an array, filters out non arrays to avoid errors, and checks for valid operation before attempting to call the operation, again to avoid throwing errors.

For every undo-able operation there is an equivalent redo operation. This means that only the operation and operation data need be stored in the undo buffer.

All objects accessible by unrelated code are frozen to ensure that state can be trusted and maintained.

Individual operations are not available via the interface but would be easy to add if needed.

const Enum = (baseId, ...names) => Object.freeze(
    names.reduce((obj, name, i) => (obj[name] = i + baseId | 0, obj), {})
);
const TextProcessor = (() => {
    function TextProcessor() {
        const undos = [];
        const undoOperations = {
            [operators.APPEND](txt) { text = text.slice(0, -txt.length) },
            [operators.BACKSPACE](txt) { text += txt },
            [operators.CLEAR](txt) { text = txt },
        };
        const  undoable = (operation, ...data) => {
            undos.length = undoPos++;
            undos.push({operation, data});
        }
        const redoUndoable = () => undoPos++;
        const operations = {
            [operators.APPEND](txt) {
                txt = "" + txt;      // forces txt to be a string
                addUndoable(operators.APPEND, txt);
                text += txt;
            },
            [operators.BACKSPACE]() {
                addUndoable(operators.BACKSPACE, text[text.length - 1]);
                text = text.slice(0, -1);
            },
            [operators.CLEAR]() {
                addUndoable(operators.CLEAR, text);
                text = "";
            },
            [operators.UNDO]() {
                if (undoPos) {
                    const op = undos[--undoPos];
                    undoOperations[op.operation](...op.data);
                }
            },
            [operators.REDO]() {
                if (undoPos < undos.length) {
                    const op = undos[undoPos];
                    addUndoable = redoUndoable;
                    operations[op.operation](...op.data);
                    addUndoable = undoable;
                }
            }
        };
        var undoPos = 0, text = "", addUndoable = undoable;
        return Object.freeze({
            get text() { return text },
            set text(txt) {
                operations[operators.CLEAR]();
                operations[operators.APPEND](txt);
            },
            process(input) {
                if (Array.isArray(input)) {
                    input = input.filter(operation => Array.isArray(operation));
                    for (const [opName, data] of input) {
                        operations[operators[opName]]?.(data);
                    }
                }
            }
        });
    }
    const operators = Enum(1, "APPEND", "BACKSPACE", "UNDO", "REDO", "CLEAR");
    return Object.freeze(Object.assign(
        TextProcessor,
        operators
    ));
})();

const textProcessor = TextProcessor()
textProcessor.process([
  ['APPEND', 'Hey'],
  ['APPEND', ' there'],
  ['APPEND', '!'],
  ['BACKSPACE'], ['BACKSPACE'], ['BACKSPACE'], ['BACKSPACE'],
  ['APPEND', '$'],
  ['UNDO'], ['REDO'],
  ['UNDO'], ['UNDO'], ['UNDO'], ['UNDO'], ['UNDO'], ['UNDO']
  ['REDO'], ['REDO'], ['REDO'], ['REDO'], ['REDO'], ['REDO'],
  ['UNDO'],
  ['APPEND', 'e'],
  ['APPEND', 'r'],
  ['APPEND', 'e'],
  ['APPEND', '$'], ['UNDO'],
  ['APPEND', '!'],
  ['CLEAR'],
  ['UNDO'],
]);

console.log("'" + textProcessor.text + "'" + " === 'Hey there!' " + (textProcessor.text === "Hey there!"));


Reading a file into string in Java I am quite new to Java, and I am trying to read a file into a string (or should I use byte arrays for this?). File can be anything, such as a text file or executable file etc. I will compress what I read and write it to another file.

I am thinking of using this code:

public static String readFile(File f) {
    StringBuilder sb = new StringBuilder();
    try (BufferedReader br = new BufferedReader(new FileReader(f))) {
        String sCurrentLine;
        while ((sCurrentLine = br.readLine()) != null) {
            sb.append(sCurrentLine);
        }

    } catch (IOException e) {
        System.err.println("I/O Exception:" + e.getMessage());
        return null;
    }
    return sb.toString();
}

Does this look good to you? I was reading "Reading and writing text files" and I was a little bit confused over all the different ways one can use to read files.

Why reinvent the wheel?

Use commons-io's FileUtils.readFileToByteArray(File)(javadoc)


Writing integer to binary file I wrote a python function that writes a integer to a file in binary form. But are there any ways that I can reduce the number of steps, is there a more efficient way to write a integer to a file?

def IntWrite(FileName, Integer):
    #convert to hexadecimal
    data = str('{0:0x}'.format(Integer))
    #pad number if odd length
    if(len(data) % 2 != 0):
        data="0"+data
    #split into chucks of two
    info = [data[i:i+2] for i in range(0, len(data), 2)]
    #convert each chuck to a byte
    data2 = "".join([chr(int(a, 16)) for a in info])
    #write to file
    file(FileName, "wb").write(data2)

#convert back to integer
def IntRead(FileName):
    RawData = file(FileName, "rb").read()
    HexData = "".join(['{0:0x}'.format(ord(b)) for b in RawData])
    return int(HexData, 16)

IntWrite("int_data.txt", 100000)
print IntRead("int_data.txt")

Python comes with a built-in pickle module for serialization:

>>> import pickle
>>> with open('data', 'wb') as f: pickle.dump(9 ** 33, f)
>>> with open('data', 'rb') as f: print(pickle.load(f))
30903154382632612361920641803529

(P.S. I wouldn't use a filename ending with .txt to store binary data.)


Identify and extract URLs from text corpus I'm working on a project that requires POS Tagging of paragraphs. The text contains lot of URLs which contain various punctuation marks such as . ?.  This affects the accuracy of the sentence tokenization.

So I decided to clean the data by removing/replacing all the URLs. And thought regular expressions would be handy in doing that. Remember, I want to match not just the domain name but the complete URL.

For example, in the below text,

Hey Mayur, We have successfully credited the cashback amount of INR 600 in your CITRUS wallet linked to mobile - 9130977755. Please download and rate our apps here - http://smarturl.it/ixigoapps Happy travelling! Team ixigo

I want the regex to match http://smarturl.it/ixigoapps

Here's what I came up with so far

https?:\/\/?[-a-zA-Z0-9@:%._\+~#=]{2,256}\.[a-z]{2,6}\b[-a-zA-Z0-9@:%_\+.~#?&=\/]*

Right now, I'm not concerned about protocols other than http and https, but I would like to match URLs without http or https but start with www. Example: www.acttv.in

What do you think of my approach so far, and the regular expression?

Have you considered:

re.compile('(www|http)\S+')

I tested it on your example string and it worked well (for both www and http). This does assume your links are likely to be links, and your data is fairly clean/consistent.