TDD svino Problem #4 - largest value from concatenated integers? From this blog post:

  Write a function that given a list of non negative integers, arranges them such that they form the largest possible number. For example, given [50, 2, 1, 9], the largest formed number is 95021.

I did this in with two tests and using a TDD style.

I am pretty unfamiliar with Java, so looking for overall feedback.  And/or inputs this fails on...

import static org.junit.Assert.*;

import java.util.Arrays;
import junit.framework.TestCase;
import org.junit.Test;

public class ProblemFourTest extends TestCase{
    ProblemFour p;
    public void setUp(){
         p = new ProblemFour();
    }

    @Test
    public void testExample() {
        assertEquals("95021",p.largestNumberFromList(Arrays.asList(50,2,1,9)));

    }
    @Test
    public void test50556() {
        assertEquals("56550",p.largestNumberFromList(Arrays.asList(50,5,56)));

    }
    @Test
    public void testComplicatedExamples() {
        assertEquals("9999129220201",p.largestNumberFromList(Arrays.asList(99,2,20,20,29,91,1,9)));
    }

}

The associated code is:

import java.util.List;

public class ProblemFour {

    public String largestNumberFromList(List<Integer> input){

        return getStringFromList(sortIntegerList(input));
    };

    private String getStringFromList(List<Integer> input){
        String s = "";
        for (int i = 0; i < input.size(); i++){
            s += input.get(i);
        }

        return s;

    }
    private List<Integer> sortIntegerList(List<Integer> input){

        for (int i = 0; i < input.size(); i++){

            for (int j = i + 1; j < input.size(); j++) {

                int a = input.get(i);
                int b = input.get(j);

                if (b == getLargestLexiographcal(a,b)){
                    input.set(i, b);
                    input.set(j, a);
                }
            }

        };

        return input;

    }

    int getLargestLexiographcal(int a, int b){

        String first = "" + a;
        String second = "" + b;

        for (int i = 0; i < Math.max(first.length(), second.length()); i++){

            int firstVal = Integer.valueOf(first.charAt(Math.min(i,first.length() - 1)));
            int secondVal = Integer.valueOf(second.charAt(Math.min(i,second.length() - 1)));

            if (firstVal != secondVal ){
                if (firstVal > secondVal){
                    return Integer.valueOf(first);
                }
                else
                {
                    return Integer.valueOf(second);
                }
            }
        }

        return 0;

    }

}

I found that the refactor stage was quite easy to do having the test cases. I dropped a bunch of duplication from getLargestLexiographcal after my second test case. It was very, very painful to write working code and not refactor once I realized I was duplicating code. I had previous treated the equal length case as different than the unequal length. However, once the test passed refactoring was very simple and straightforward - a compelling reason for me to not refactor when trying to pass a test, and only afterwards. It lets you incremental refactor, rerun tests, repeat very quickly.

Style - good

The TDD is a great way to start. You can see the benefits already, especially in the fact that it was easy for me to copy your code and run it locally, and make sure it works (not that I doubted it, but it's nice to check).

There's a typo in a method name... I know that's a silly thing to point out, but it is the first thing I saw. My spelling (actually, my typing) is often criticised, and as a result I am sensitive to these things. The method getLargestLexiographcal is missing an i here: getLargestLexiographical.

Beyond that, though, the code is really neat, and well structured. Two nit-picks.... in your bracing style (yeah, yeah, I know, not the bracing....!) But, you should be consistent with the space before the opening brace. Sometimes you have a space before the brace, and sometimes, not. You should have a space. So, for example, the lines:

        if (firstVal != secondVal ){
            if (firstVal > secondVal){

should be:

        if (firstVal != secondVal) {
            if (firstVal > secondVal) {

The other nitpick is the newlines in the else block. Yeah, it's a real nitoick, but I prefer a single-line for the } else {, not 3 lines. Still, there is only one instance of this, so at least it is consistent ;-)

OK, enough about the trivials....

Minutia

String concatenation to get a String value of an Integer is overkill... things like:

 String first = "" + a;

Really, that should be:

 String first = a.toString();

The Integer.valueOf(...) in the following line should also probably be Character.getNumericValue(...):

Integer.valueOf(first.charAt(Math.min(i,first.length() - 1)))

should be:

Character.getNumericValue(first.charAt(Math.min(i,first.length() - 1)))

Sorting

Your sorting algorithm is simplistic, but effective. A bubble-sort, of sorts. The major drawback I see is the many comparisons, especially given that each comparison is performed from first-principles... calculating the lexographical value each time, instead of memoizing it.

All told, though, I really would have preferred to see a standard-library sort using a Comparator for the comparisons.... The comparator would look something like:

private static final Comparator<Integer> LEXIOGRAPHIC = new Comparator<Integer>() {

    @Override
    public int compare(Integer a, Integer b) {
        // same values compare the same.
        if (a.equals(b)) {
            return 0;
        }
        String first = a.toString();
        String second = b.toString();

        for (int i = 0; i < Math.max(first.length(), second.length()); i++){

            int firstVal = Character.getNumericValue(first.charAt(Math.min(i,first.length() - 1)));
            int secondVal = Character.getNumericValue(second.charAt(Math.min(i,second.length() - 1)));

            if (firstVal != secondVal ){
                if (firstVal > secondVal){
                    return -1;
                } else {
                    return 1;
                }
            }
        }

        return 0;
    }

};

Then, with that comparator, you can just sort the list:

public String largestNumberFromList(List<Integer> input){

    input.sort(LEXIOGRAPHIC);

    return getStringFromList(input);
};

The sort used will be the native sort in Java, an efficient TimSort, I believe.

When possible, use the library functions available....

Alternative

I would recommend creating a number class, that implements Comparable, then, when you process the system, just create a new instance of that class for each input value. The constructor should create the lexographical value at that time, and the comparator will then be much simpler too. If you add the values to an always-sorted collection, like a TreeSet, then you can just do it all in one go.

Hmmm... in retrospect, I played around with it, and came up with the following class, and a 'simple' lambda expression to arrange it.... Note, the compareTo function is a 'clever' trick of appendign each value in opposite orders, and then comaring them. It makes the relative order easy to compute. It may not be the fastest, but it sure is simple to read....

private static final class LexiVal implements Comparable<LexiVal> {

    private final String value;

    public LexiVal(int value) {
        this.value = Integer.toString(value);
    }

    @Override
    public int compareTo(LexiVal o) {
        final String usthem = value + o.value;
        final String themus = o.value + value;
        return themus.compareTo(usthem);
    }

}

public String largestNumberFromList(List<Integer> input){
    return input.stream()
                .map(i -> new LexiVal(i))
                .sorted()
                .map(x -> x.value)
                .collect(Collectors.joining());
};

Aas a side note, that same logic used in the LexiVal could be used in the Comparator instead.....

private static final Comparator<Integer> LEXIOGRAPHIC = new Comparator<Integer>() {

    @Override
    public int compare(Integer a, Integer b) {
        String ab = a.toString() + b.toString();
        String ba = b.toString() + a.toString();
        return ba.compareTo(ab);
    }

};

then your algorithm becomes:

public String largestNumberFromList(List<Integer> input){
    input.sort(LEXIOGRAPHIC);
    return getStringFromList(input);
};

Additionally, the getStringFromList(input) can be made in to a Jav8 stream, with:

return input.stream().map(i -> i.toString()).collect(Collectors.joining());


Writing a better deploy script This is a script I've written to deploy the Elastic Stack.

Can you help me improve it? Especially the parts called out with *** markings.

deploy.sh:

#!/usr/bin/env bash
sudo sysctl -w vm.max_map_count=262144
#use the elastic utility to gen certs
docker-compose -f create-certs.yml run --rm create_certs
# Start the stack initially per Elastic documentation
docker-compose up -d
#run a password gen script to make some passwords
docker exec elasticsearch /bin/bash -c "bin/elasticsearch-setup-passwords \
auto --batch \
--url https://elasticsearch:9200" | tee es_passes.txt
#sub generated kibana and es passes into .env -> *** This could be better ***
cat es_passes.txt  | grep 'PASSWORD kibana' | awk '{print $4}' | xargs -I {} sed -r -i 's/(KIBANA_PASS=)\w/\1{}/gip' .env
cat es_passes.txt  | grep 'PASSWORD elastic' | awk '{print $4}' | xargs -I {} sed -r -i 's/(ES_PASS=)\w/\1{}/gip' .env
#eliminate duplicate lines -> *** How can I do this better ***
awk '!seen[$0]++' .env > .env_dedup
mv .env_dedup .env
#show the contents of .env
cat .env
#restart the stack after setting the passwords
docker-compose stop
docker-compose up -d

.env:

KIBANA_PASS=some_supersecure_kibana_pass
ES_PASS=some_supersecure_elasticsearch_pass

other files:

docker-compose.yml
create-certs.yml

update:
I have fixed the substitution lines. They now read:

cat es_passes.txt  | grep 'PASSWORD kibana' | awk '{print $4}' | xargs -I {} sed -r -i 's/^KIBANA_PASS=.*$/KIBANA_PASS={}/' .env
cat es_passes.txt  | grep 'PASSWORD elastic' | awk '{print $4}' | xargs -I {} sed -r -i 's/^ES_PASS=.*$/ES_PASS={}/' .env

Shots from the hip, three decades since I seriously programmed shell notwithstanding:

document your code - in the code.
The comments presented are a good start - in the middle, irritatingly.
What is the whole script useful for, how shall it be used?
You can even have executable comments:
if called incorrectly or with -h, --help (or even -?), print a
Usage: to "standard error output"/file descriptor 2
Then there are things left open:
Why tee to es_passes.txt instead of redirect, have sed print the pattern space?  (I read Spanish_passes at first - the reason why in the following comment es may be as appropriate as elastic needed horizontal scrolling)
with security related artefacts, assess security implications
choose the right tool
this does not just depend on task at hand and tools available, but on "craftsperson", too
(Here, I might have chosen perl in the 90ies and Python in the current millenium century)
the options and commands to sed speak of GNU sed
• mention such in your question
• consider using --posix
don't use cat | command where command allows specifying input file(s)
prefer using awks patterns over separate filtering (e.g.,grep)
(unless input is massive):
awk '/PASSWORD kibana|elastic/{print $4}' es_passes.txt …
(not convinced piping to xargs -I is the way to proceed.)
the conventional "unix" way to eliminate duplicate lines is uniq, preceded with a sort where global uniqueness is necessary and altering line sequence admissible.


Promise Chaining Forgot Password I have a route I've rewrote to use promises.

router.post(
  '/forgot-password',
  (req, res) => {
    //generate random password
    let passwordString = Math.random().toString(36).slice(-10);

    User
      .findOne({'username': req.body.email})
      .then(foundUser => {
        foundUser.setPassword(
          passwordString,
          () => {
            foundUser.save(
              () => {
                emailService.sendForgotPassword(foundUser, passwordString);
                req.flash('success', 'Please check your email.');
                res.redirect('/login');
              });
          });
      })
      .catch(err => {
        if(err && err.message) req.flash('error', 'Cannot find a user by that email.');
        res.redirect('back');
      });
  });

I think my then route could be better than what it is now. How do I optimize the promise chain a little better?

You can't optimize a promise chain that doesn't exist. The only method that's returning a promise here is User.findOne(). In order for the route to be promise based, you'll need to rewrite the other methods you are calling inside the route.

I'll do my best, with the small amount of information you provided, to help you figure out how to change it.

foundUser.setPassword() seems to be asynchronous but it's using the callback pattern: foundUser.setPassword( newPassword, functionToCallWhenDone ). You need to rewrite it so instead of accepting a function to call back once done, it should return a promise as soon as it finishes or errors out. That way it can be chained with the other promises in the route.

The same can be said about foundUser.save( functionToCallWhenDone ).

Also emailService.sendForgotPassword( foundUser, passwordString ) seems to be crucial to the request's success. I recommend you to rewrite it so it returns a promise too, and catch the error outside the main chain to handle it.

After applying these changes your code could look like this:

// Instead of using comments to indicate your intention, abstract the logic into a method
function generateRandomPassword() {
  return Math.random().toString( 36 ).slice( - 10 );
}

router.post(
  '/forgot-password',
  ( req, res ) => {
    let newPassword;
    let user;

    // I like to start with Promise.resolve() as that makes all the other method calls be at the same indentation level
    // Note: This is not required and purely just about style
    // Also, wrapping anything you'll do around a promise's then method protects you from unforeseen errors
    Promise.resolve()
      // TODO: What happens if the email wasn't sent with the request?
      .then( () => User.findOne( { 'username': req.body.email } ) )
      .then( _user => {
        user = _user;

        newPassword = generateRandomPassword();
        return user.setPassword( newPassword );
      } )
      .then( () => user.save() )
      .then( () => emailService.sendForgotPassword( user, newPassword )
        .catch( error => {
          // What happens if the emailService fails?
          // Here you might want to revert changing the user's password
          revertPasswordChange();

          // Don't forget to return a rejected promise (or throw again the error) because if not, the next then will wrap
          // whatever you return here (or an empty value if you didn't return something) and the success message will be
          // wrongly displayed.
          return Promise.reject( error );
        } )
      )
      .then( () => {
        req.flash( 'success', 'Please check your email.' );
        res.redirect( '/login' );
      } )
      .catch( error => {
        if( error && error.message ) req.flash( 'error', error.message );
        res.redirect( 'back' );
      } )
    ;
  }
);

Note that this code relies on mutable contextual state, which has some drawbacks and (depending on the context) may be considered an anti-pattern. Here's a very good read about how to share state between promises.


Java methods to add and authenticate users in MySQL I'm going through some code and trying to figure out how to get rid of duplicates from the following two methods. Both methods execute a simple login/register query to a local MySQL DB. I couldnt figure out a good design pattern to solve my problem, especially to get rid of the duplicate try-catch.

Any advice?

public class DatabaseHandler {

    [...]

    public static boolean checkLogin(String username, String password) {
        Connection connection = createConenction();
        PreparedStatement statement = null;

        String query = "select * from users where username = ? and password = ? ";

        try {
            statement = connection.prepareStatement(query);
            statement.setString(1, username);
            statement.setString(2, password);
            ResultSet result = statement.executeQuery();
            return result.next(); //True if User exists
        } catch(SQLException e) {
            return false;
        } finally {
            try {
                statement.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }
    }

    public static boolean registerUser(String username, String password) {
        Connection connection = createConenction();
        PreparedStatement statement = null;

        String query = "insert into users (username, password) values (?, ?)";

        try {
            statement = connection.prepareStatement(query);
            statement.setString(1, username);
            statement.setString(2, password);
            statement.executeUpdate();
            return true;
        } catch(SQLException e) {
            return false;
        } finally {
            try {
                statement.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }
    }
}
```

Funny coincidence, I just created the solution for your problem (but in PHP). The common thing here is the one-time use of a prepared statement, the difference is in the query string and arguments, so what you need is a function that accepts a query string, an argument determining what execution function to use (executeQuery() or executeUpdate()) and the parameters for the query:

// note: QueryType is an enum you should define
public static ResultSet query(String sql, QueryType queryType, Object... queryArgs){
    // ...
}

Include the try-catch blocks in this function too of course, and return null when according to queryType there should be no results.

Note: I have no experience with database interaction with Java, so if there are only 2 different statement execution functions then you can use a boolean argument instead of the enum QueryType.

Then your code could look like this:

public static boolean checkLogin(String username, String password) {
    String sql = "select * from users where username = ? and password = ? ";
    ResultSet result = query(sql, QueryType.RETURN_RESULT, username, password);
    return result.next(); // true if User exists
}

A note about your query string: I find it more convenient to use CAPS for keywords and lowercase text for names, it highlights the structure of the query.


Changing http-requests conditionally I'm new to RXJS and I have a working piece of code for http-request and subscribing to Observables from a shared service in Angular. This works, but I'd want to get rid of the nested subscriptions, which I know is probably very possible with RXJS operators, but I haven't figured out how. The usage of flatMap in the nested request wouldn't be necessary, as the second nested request isn't dependable on the first.

  this.sub = this.service.isPending$
    .subscribe(data => {
      if(data) { // gives boolean
        this.service.getPending()
          .subscribe((data:Offer) => this.offer = data)
      } else {
        this.sub2 = this.service.amount$
        .flatMap((data: Amount) => {
          this.amount = data;
          return this.service.getOffer()
        })
        .subscribe((data: Offer) => {
          this.offer = data;
        })
      }
    })

How would I go about cleaning this code up? I'm using RXJS: "^5.0.1"

On Observables

My personal preference is to have as few Subscription objects and subscribe() invocations as possible. If I don't have mistakes in the refactored code, you can achieve a single subscribe() which does the same things as your code. I also got rid of sub2.

this.offerSubscription = this.service
    .isPending$
    .flatMap(data => {
        this.amount = null;
        const offerObservable = data ?
            this.service.getPending() :
            this.service
                .amount$
                .flatMap((data: Amount) => {
                    this.amount = data;
                    return this.service.getOffer();
                });
        return offerObservable;
    })
    .subscribe((offer: Offer) => this.offer = offer);

On TypeScript

sub, sub2, isPending$ and data are pretty bad names because they don't describe the entities they hold. I recommend changing them.

At the bare minimum, specify the type of the input parameter in your arrow function to make the code more readable: (data: Offer) => {...} or better (receivedOffer: Offer) => { ... }.


Using callbacks in a Restangular service I have a project built with AngularJS and using Restangular in my services to handle API requests. The problem is I haven't found a good way to test the services which I think has turned into a bit of a code smell for me. I'm using callbacks in the promise to handle pushing the data back to the controller instead of just returning the result.

Here is a simple controller and service:

myApp.controller('CustomerController', ['$scope', 'Teams', function($scope, Teams) {
    Teams.getList(function(results) {
        $scope.teams = result;
    });
}]);
myApp.factory('Teams', ['Restangular', function(Restangular) {
    var Teams = {};
    Teams.getList = function(callback) {
        Restangular.all('teams').getList().then(function(results) {
            callback(results);
        });
    };
    return Teams;
}]);

Is that really the best way to get the data back to the controller? I'm starting to feel like this isn't the correct way to do it but I haven't found another way that works.

Well, when you're already using promises you also just return that instead of using a callback.  Then you can also chain more promises at the end and add error handling.  Other than that it looks fine; I don't see what you'd really change there.

myApp.controller('CustomerController', ['$scope', 'Teams', function($scope, Teams) {
    Teams.getList().then(function(results) {
        $scope.teams = result;
    }, function(error) {
        ...
    });
}]);
myApp.factory('Teams', ['Restangular', function(Restangular) {
    var Teams = {};
    Teams.getList = function() {
        return Restangular.all('teams').getList();
    };
    return Teams;
}]);

If you want to get rid of Teams, I've seen this variant as well:

myApp.factory('Teams', ['Restangular', function(Restangular) {
    var getList = function() {
        return Restangular.all('teams').getList();
    };

    return {
        getList: getList
    };
}]);


Passing an entity to use it's fields to update method is ok to do? I have two different ideas on how to handle this Update method in my MVC Model class "DataAccess" And I'm wondering if in the first, I'm handling my class incorrectly or ambiguously, because I'm using members from the same object for two different purposes.

The man Class has 2 relevant members.  ID, which is the primary key of the entity, and Name, which is the description

    public DbResult Update(Man man)
    {
        DbResult dbResult;

        var query =
            from person in _MyDBEntities.Men
            where person.ManID == man.ManID // Man with ManID is what we're looking for.
            select person;

        try
        {
            foreach (Man m in myQuery)
            {
                m.Name = man.Name; // man.Name is the new name
            }
            dbResult = DbResult.Success("Record updated");
        }
        catch (Exception e)
        {
            dbResult = DbResult.Failed(e.ToString());
        }
        return dbResult;
    }

And this version of the method, which uses the variables passed in differently.

    public DbResult Update(int idToLookFor, string newName)
    {
        DbResult myResult;

        var query =
            from person in _MyDBEntities.Men
            where person.ManID == idToLookFor
            select person;

        try
        {
            foreach (Man m in myQuery)
            {
                m.Name = newName;
            }
            dbResult = DbResult.Success("Record updated");
        }
        catch (Exception e)
        {
            dbResult = DbResult.Failed(e.ToString());
        }
        return dbResult;
    }

is my first method a better or ok approach to the problem?  Because what if I have an update method later on that uses 8 members, and need to create more versions of this function for updating other entities?

## UPDATED ##

Including a new class here:

public class InputValidation//<T> where T: class
{
    /// <summary>
    /// Delegate that matches the signature of TryParse, method defined for all primitives.
    /// </summary>
    /// <typeparam name="T">Output type of This Delegate</typeparam>
    /// <param name="input">input for this Delegate to translate to type T</param>
    /// <param name="output">The translated variable to return via out parameter</param>
    /// <returns>Whether the Parse was successful or not, and output as output</returns>
    public delegate bool TryParse<T>(string input, out T output);
    public bool ValidateInputType<T>(string input, TryParse<T> TypeValidator, out T result)
    {
        return TypeValidator(input, out result);
    }
    public bool ValidateInputRange<T>(ref T result, int lower, int upper)
    {
        // How can I use relational operators like > < = on T? without forcing T result to an int?
        return isValidRange(int.Parse(result.ToString()), lower, upper);
    }
    public bool isValidRange(int item, int lower, int upper)
    {
        return (lower <= item && item <= upper);
    }
}

The second one seams to me a better approach but you say that the ManID is a primary key, so why are you handling it as a non primary key and using the query result as a sequance?

public DbResult Update(int idToLookFor, string newName)
{
    return Update(_MyDBEntities.Men.Single(p => p.ManID == idToLookFor), newName);
}

public DbResult Update(Man person, string newName)
{
    try
    {
        person.Name = newName;
        //saving missing?
        return DbResult.Success("Record updated");
    }
    catch (Exception e)
    {
        return DbResult.Failed(e.ToString());
    }
}

And the real saving missing or it's placed to elsewhere so why are you setting here the DbResult?


Sequence Generator from previous max sequence based on certain conditions I'm new to VBA programming so the following code may look terrible which is why I want ways to understand how to better optimize it and other future codes.

Sub Sequence()
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
Application.DisplayAlerts = False
Dim unique_string1, unique_string2, unique_string3 As String

Dim arrj() As String
Dim arrk() As String
Dim arro() As String
Dim a, b, c As Long

'ARRAYj

jr = Sheets("Docs").Cells(Sheets("Docs").Rows.Count, 1).End(xlUp).Row
Set range1 = Sheets("Docs").Range("J2:J" & jr)
For Each cel In range1
    If Not InStr(unique_string1, cel.Value) > 0 Then
        unique_string1 = unique_string1 & cel.Value & ","
    End If
Next
arrj = Split(unique_string1, ",")

'ARRAYk

kr = Sheets("Docs").Cells(Sheets("Docs").Rows.Count, 1).End(xlUp).Row
Set range2 = Sheets("Docs").Range("K2:K" & kr)
For Each cel In range2
    If Not InStr(unique_string2, cel.Value) > 0 Then
        unique_string2 = unique_string2 & cel.Value & ","
    End If
Next
arrk = Split(unique_string2, ",")

'ARRAYo

pr = Sheets("Docs").Cells(Sheets("Docs").Rows.Count, 1).End(xlUp).Row
Set range3 = Sheets("Docs").Range("o2:o" & pr)
For Each cel In range3
    If Not InStr(unique_string3, cel.Value) > 0 Then
        unique_string3 = unique_string3 & cel.Value & ","
    End If
Next
arro = Split(unique_string3, ",")

'ForLoop

For a = LBound(arrj) To UBound(arrj)
    For b = LBound(arrk) To UBound(arrk)
        For c = LBound(arro) To UBound(arro)
            If Not (arrj(a) = "" Or arrk(b) = "" Or arro(c) = "") Then

                Dim x, y, z As String
                x = Trim(arrj(a))
                y = Trim(arrk(b))
                z = Trim(arro(c))

                Dim lRow   As Long, i As Long
                Dim ws     As Worksheet
                Dim nextn  As Long
                Dim getAddress As String
                Set ws = ActiveSheet

                With ws
                    lRow = .Cells(.Rows.Count, 1).End(xlUp).Row

                    For i = 2 To lRow
                        If .Cells(i, 10).Value = x And .Cells(i, 11).Value = y And .Cells(i, 15).Value = z And .Cells(i, 23).Value = "" Then
                            getAddress = .Cells(i, 23).Address
                            nextn = 1 + Application.Evaluate("MAX(IF((J2:J20000=""" & x & """)*(K2:K20000=""" & y & """)*(O2:O20000=""" & z & """),INT(w2:20000)))")
                            Range(getAddress).Value = nextn
                        End If
                    Next i
                End With
            End If
        Next c
    Next b
Next a
Application.ScreenUpdating = True
Application.DisplayAlerts = True
Application.Calculation = xlCalculationAutomatic
End Sub

So the code gets the distinct value from the columns and stores it in an array. Then those 3 arrays containing the distinct value combination is used to check for the max of the sequence and assigned a plus 1 to the max sequence if found. Else a 1.

How could I make this run faster considering there are a lot of rows and combinations to check?

P.s: If you have a code for getting distinct value from a column and store it into and array it would be great.

Mock-DATA

   J     K     O         W
1jobid  dep  job_no   Sequence
2 aaa   FJ     1         1      <existing data>
3 aaa   FJ     1         2
4 aaa   FJ     1         5
5 aaa   RJ     1         1
6 aaa   RJ     1         9
7 aaa   RJ     1         10     <existing data>
-----------------------------------------------------
8 aaa   RJ     1         11     <after the code is run, 11 is assigned>
9 aaa   FJ     1          6     <after the code is run, 6 is assigned >

columns J,K,O are strings and column W is number.

EDIT : As mentioned in the comments changed Range("J2:J20000" & jr) to Range("J2:J" & jr)

Dim unique_string1, unique_string2, unique_string3 As String

Variables declared without the As keyword are declared as a variant.  In the line above unique_string3 is declared as a String, unique_string1 an unique_string2 are variants.

The original posted code use Range("J2:J20000" & jr).  jr was being concatenated to the end of "J2:J20000".  The post has been edited and corrected.

W is missing from the formula

INT(w2:20000))

getAddress = .Cells(i, 23).Address
Range(getAddress).Value = nextn

Use .Cells(i, 23).Value = nextn instead.  This will qualify the ranges to the worksheet instead of the ActiveSheet,

Sheets("Docs").Evaluate("MAX(IF((J2:J20000=""" & x & """)*(K2:K20000=""" & y & """)*(O2:O20000=""" & z & """),INT(w2:20000)))")

It is better to change the worksheets codenames for easy reference.
VBA Codenames:
Sheet (document) modules have a property called CodeName, which is how the object is know internally to VBA. Indeed, if this feature is used by the developer, VBA code can always refer to a sheet (worksheet or chart), even if the sheet is renamed from Excel by a user. For example:

A worksheet is named 'Sales-2012'
Sheet CodeName is 'Sales'
User renames the worksheet to 'Sales-2013'

Use the Worksheet's Evaluate method instead of the Application's.

Refactored Code

Here is a rough rewrite of the OP's code:

Option Explicit

Sub NewSequence()
    Dim JobIds As Variant
    JobIds = GetUniqueValues("J")

    Dim Deps As Variant
    Deps = GetUniqueValues("K")

    Dim Job_Nos As Variant
    Job_Nos = GetUniqueValues("O")

    With wsDoc
        Dim LastRow As Long
        Rem Make sure that this refers to the longest used column
        LastRow = .Cells(.Rows.Count, 1).End(xlUp).Row

        Const BaseFormula As String = "MAX(IF((J2:J@LastRow=""@jobid"")*(K2:K@LastRow=""@dep"")*(O2:O@LastRow=""@job_no""),INT(w2:w@LastRow)))"

        Dim Formula As String
        Dim Id As Long
        Dim Department As Long
        Dim JobNumber As Long
        Dim i As Long

        For Id = 0 To UBound(JobIds)
            For Department = 0 To UBound(Deps)
                For JobNumber = 0 To UBound(Job_Nos)
                    For i = 2 To LastRow
                        Formula = Replace(BaseFormula, "@LastRow", LastRow)
                        Formula = Replace(Formula, "@jobid", JobIds(Id))
                        Formula = Replace(Formula, "@dep", Deps(Department))
                        Formula = Replace(Formula, "@job_no", Job_Nos(JobNumber))
                        .Cells(i, 23) = 1 + .Evaluate(Formula)
                    Next
                Next
            Next
        Next

    End With
End Sub

Private Function GetUniqueValues(ColumnName As Variant) As Variant
    Dim Target As Range
    With wsDoc
        Set Target = .Range(.Cells(2, ColumnName), .Cells(.Rows.Count, ColumnName).End(xlUp))
    End With

    Rem If there are no values then exit the function
    Rem The calling method should test if the return value is an array using isArray()
    If Target.Row = 1 Then Exit Function

    Rem Range.Value will return an 1 based range of values it the Range contains multiple cells
    Rem A Range that contains a single cell then it will return a single scalar value
    If Target.Count = 1 Then
        Rem Return the single value wrapped in a zero based array
        GetUniqueValues = Array(Target.Value)
        Exit Function
    End If

    Rem Typically a Scripting.Dictionary is used to return an unique lists
    Dim list As Object
    Set list = CreateObject("Scripting.Dictionary")

    Rem assign an 1 based array of values from the Target range to an array
    Dim Values As Variant
    Values = Target.Value

    Dim Item As Variant

    Rem Use For Each controls to iterate over the Values array
    For Each Item In Values
        Item = Trim(Item)
        If Not list.Exists(Item) And Len(Item) > 0 Then list.Add Key:=Item, Item:=Item
    Next

    Rem Return a zero based array from the Dictionary
    GetUniqueValues = list.Keys
End Function

I changes the Doc tab codename to wsDocs for the code above.  Thanks Peter T!


Creating a script to automate implicit remoting Goal:

I am attempting to create a script that would automatically establish a PSSession to a Windows server for implicit remoting.

Problem:

Export-PSSession : Proxy creation has been skipped for the '%' command, because Windows PowerShell could not verify the safety of the command name.
At Z:Somewhere\aScript.ps1:12 char:3
+   Export-PSSession -Session $ServerPS -OutputModule 'First Module' - ...
+   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidData: (:) [Export-PSSession], InvalidOperationException
    + FullyQualifiedErrorId : ErrorSkippedUnsafeCommandName,Microsoft.PowerShell.Commands.ExportPSSessionCommand

Side notes:

A large number of users will use this script to establish PSSessions daily, I am trying to find the most efficient way to get all the users to access these modules - Amodule, Bmodule and Cmodule. Additionally, the script still works despite the above mentioned error.

Here is a sample of my script:

$Server = New-PSSession -ComputerName ServerPS -Authentication Kerberos
Set-Alias -Name go -Value Get-CustomMods -Description 'Gets the modules'
function Get-CustomMods
{
  # Import first module
  Write-Verbose -Message 'Importing First module'
  Invoke-command { import-module 'Amodule.ps1' } -session $ServerPS
  Export-PSSession -Session $Server -OutputModule 'Amodule' -Force -AllowClobber
  # Import second module
  Write-Verbose -Message 'Importing Second Module'
  Invoke-command { import-module 'Bmodule.ps1'} -session $ServerPS
  Export-PSSession -Session $Server -OutputModule 'Bmodule' -Force -AllowClobber
  # Import third module
  Write-Verbose -Message 'Importing Third Module'
  Invoke-command { import-module 'Cmodule.ps1' } -session $ServerPS
  Export-PSSession -Session $Server -OutputModule 'Cmodule' -Force -AllowClobber
}

Question:

Is there a more effective way to structure/write this script given my goal and problem?

I wish I had more to offer but I wanted to add some pointers about the code I do see.

Code Repetition

If you find yourself repeating the same code over and over again you should be asking if there is another way.

Another function

You could easily wrap the block

  # Import first module
  Write-Verbose -Message 'Importing First module'
  Invoke-command { import-module 'Amodule.ps1' } -session $ServerPS
  Export-PSSession -Session $Server -OutputModule 'Amodule' -Force -AllowClobber

into its own function. That way if you need to make some changes they will be more centralized.

function Export-SessionModule{
    param(
        $session,
        $moduleName
    )
    # Import named module
    Write-Verbose -Message "Importing $moduleName module"
    Invoke-command { import-module "$moduleName.ps1" } -session $session
    Export-PSSession -Session $session -OutputModule $moduleName -Force -AllowClobber
}

Splatting

Note that this is meant to be a simple example. Things like parameter typing might be advisable but were omitted for brevity. If functions are overkill for this little script of yours something else to consider would be splatting your parameters that are the same across cmdlet calls.

$exportPSSessionParams = @{
    Session = $session
    Force = $true
    AllowClobber = $true
}

# .....
Export-PSSession -OutputModule 'Amodule' @exportPSSessionParams
# .....
Export-PSSession -OutputModule 'Bmodule' @exportPSSessionParams
# .....
Export-PSSession -OutputModule 'Cmodule' @exportPSSessionParams

Variable Names

You follow the verb-noun convention for your functions which is good.

Your session variable name is a little misleading. You call it $server. I usually see that representing a string with a server name. Since it is a session object it would be clearer if it was named as such. $session at a minimum or even $serverSession if you prefer. This could clash with your $sessionPS which is more reason to make them clear.

Function Declaration

You are supposed to declare your functions before you use them. That being said, you would typically find functions at the beginning of scripts. While the order you have things declared will not be a problem (since it is working) I wanted to be sure that was understood.

If you were to call go after your created the alias it would fail.

Your Problem

Like I said I don't have much to offer here. I would like to point out that I think you scrubbed your code incorrectly since the code in your error does not match your code block.

Have you checked on SO for that message already? There are a few different causes although I didn't find an exact match. Admittingly I do not work with modules much.
I would debug this by seeing if it is the same module and if perhaps how it is being called is causing some problems. Perhaps being run more than once in a session? (AllowClobber should have fixed that though)


iterative copy-tree in lisp The common lisp function copy-tree is often implemented in a recursive way,
and thus is prone to stack overflow.

Here is my attempt at writing an iterative version, one-pass, no stack,
no unnecessary consing, version of copy-tree in common-lisp, working
with nested, perhaps dotted, lists.

 (defun my-copy-tree (tree)
     (do* ((result (list tree))
           (node result)
           (next))
          ((null node) result)
       (cond ((and (consp (cdar node))(atom (caar node)))
              (setf (cdr node) (cons (cdar node) (cdr node))
                    (car node) (caar node)
                    node (cdr node)))
             ((consp (cdar node))
              (setf (cdr node) (cons (cdar node) (cdr node))
                    (car node) (cons (caar node) (cdr node))
                    node (car node)))
             ((consp (caar node))
              (setf (car node) (cons (car node) (cdr node))
                    (cdr node) (cdaar node)
                    node (car node)
                    (car node) (caar node)))
             (t (setf next (cdr node)
                      (cdr node) (cdar node)
                      (car node) (caar node)
                      node next)))))

The performance is ok. In fact, on sbcl it is faster than the
native implementation, and almost as fast as copy-list for proper lists.
It also has no problem making copies of "reversed" lists like the one given by

(reduce 'cons (make-list 1000000 :initial-element 0))

(by the way, are there any lisp implementation with a native copy-tree
 that does not stack-overflow on these kind of deep lists ?)

Unfortunately, my code is unreadable, and I must say I can't really understand
it now without drawing a whole page of cons cells. Also it is not very flexible,
e. g. there is no obvious way to turn it into a reduce-tree, in contrast
to the recursive version.

How to rewrite the code above in a more readable and flexible way, and
so to speak in a more lispy way, without losing performance ?

Writing an efficient implementation of copy-tree in lisp should be a standard exercice but google didn't bring anything relevant.

RE: how to rewrite to make it more readable

Stop using c[a,d]*r. While I have a soft spot for them your code may be more readable via first, second etc.
Name the predicates of the cond statement. What are they testing for? Use flet or labels.
Perhaps name the setf blocks to make it more clear what they are doing.
Perhaps split the setf blocks into separate setf lines. I had to check the hyperspec to verify that in fact each one was evaluated sequentially. It would be clearer if they were explicitly sequential.

To summarize:

improve naming
make things explicit

Re: performance

Without having performance benchmarks I cannot tell how the above changes would change the performance of the code. I also noticed that you didn't specify a optimize declaration. That could be used to change performance.


Displaying TimeSpan as largest interval (with units) The following method is used in a call center application to display an approximation of remaining time. The call center telephone operator would inform the caller that they could perform their desired action (like, for example, place another order) after N time has elapsed. You'll note some arbitrary rules revolving around the interpretation of time embedded within the method, e.g., an interval greater than 50 minutes is considered to be "1 hour"; these business rules need to remain as is.

With that in mind, are there any suggestions on how this routine might be enhanced and/or improved (i.e., made more efficient)?

N.B. Obviously, the millisecond case (the "else") is not practical, but only included now for the sake of completeness. It may, in the end, be converted to something like 'less than 3 seconds' equals "now". For the sake of this question, however, let's go with the routine as written.

public static string LargestIntervalWithUnits(TimeSpan interval)
{
    if (interval < TimeSpan.Zero)
    {
        throw new ArgumentOutOfRangeException("interval");
    }

    if (interval == TimeSpan.Zero)
    {
        return "now";
    }

    int timeValue;
    string timeUnits;

    if (interval.TotalHours > 22.0)
    {
        timeValue = (int)Math.Ceiling(interval.TotalDays);
        timeUnits = " day";
    }
    else if (interval.TotalMinutes > 50.0)
    {
        timeValue = (int)Math.Ceiling(interval.TotalHours);
        timeUnits = " hour";
    }
    else if (interval.TotalSeconds > 40.0)
    {
        timeValue = (int)Math.Ceiling(interval.TotalMinutes);
        timeUnits = " minute";
    }
    else if (interval.TotalMilliseconds > 500.0)
    {
        timeValue = (int)Math.Ceiling(interval.TotalSeconds);
        timeUnits = " second";
    }
    else
    {
        timeValue = (int)Math.Ceiling(interval.TotalMilliseconds);
        timeUnits = " millisecond";
    }

    return string.Format("{0:#,##0}{1}{2}",
                         timeValue,
                         timeUnits,
                         (timeValue == 1 ? string.Empty : "s"));
}

Edit: Subsequent refactored code block, that was added after answers were posted, has been removed per users' suggestions. (This note has been added to clarify comments posted by users.)

Edit 2: The code listed above has been refactored and reposted for further review: Displaying TimeSpan as largest interval (with units) [Part II]

The numbers 22.0, 50.0, 40.0, and 500.0 are "magic numbers" (non-obvious hard-coded numbers with no given meaning).

Since they're not commonly used with time, thus are not obvious, you should make the reader aware of their meaning.  This will also improve readability.

Two options for doing this include:

making them into constants with appropriate names

private static readonly double someConstant = 22.0;
// remaining constants

providing comments or full documentation on their meaning


Creating a shortcut class in C# I'm working on making a shortcut... manager, if you will. It will be reused throughout the rest of the application for registering types that can later be resolved into views. I have a rule that the shortcuts need to be able to be grouped by their different purposes throughout the application. For instance, if I have shortcuts for car related stuff and then shortcuts for truck related stuff I would need them sorted.

This is what I have come up thus far. Thoughts, comments, and suggestions on changing this to make it better would be appreciated.

public static Shortcuts
{
    public enum ShortcutType
    {
        Car, Truck
    }

    static Shortcuts
    {
        AllShortcuts = new Dictionary<string, Type>();
        CarShortcuts = new Dictionary<string, Type>();
        TruckShortcuts = new Dictionary<string, Type>();
    }

    public static Dictionary<string, Type> AllShortcuts { get; private set; }
    public static Dictionary<string, Type> CarShortcuts { get; private set; }
    public static Dictionary<string, Type> TruckShortcuts { get; private set; }

    public static void AddShortcut(KeyValuePair<string, Type> kvp, ShortcutType type)
    {
        switch(type)
        {
            case ShortcutType.Car:
                CarShortcuts.Add(kvp.Key, kvp.Value);
                break;
            case ShortcutType.Truck:
                TruckShortcuts.Add(kvp.Key, kvp.Value);
                break;
        }

        if (!AllShortcuts.Contains(kvp))
        {
            AllShortcuts.Add(kvp.Key, kvp.Value);
        }
    }

    public static void RemoveShortcut(KeyValuePair<string, Type> kvp, ShortcutType type)
    {
        switch(type)
        {
            case ShortcutType.Car:
                if (CarShortcuts.Contains(kvp))
                {
                    CarShortcuts.Remove(kvp.Key);
                }
                break;
            case ShortcutType.Truck:
                if (TruckShortcuts.Contains(kvp))
                {
                    TruckShortcuts.Remove(kvp.Key);
                }
                break;
        }

        if (AllShortcuts.Contains(kvp))
        {
            AllShortcuts.Remove(kvp.Key);
        }
    }
}

Here are some thoughts to ponder:

If you are to access a car short cut, then you would need to know:
(i) that you want to get a car short cut, and (ii) you would have to call the relevant property, and (iii) presumably you’d have to enumerate till you found the shortcut of your choice.
There seems to be some duplication: values are being stored in at least two dictionaries: (i) either the car or truck short-cut dictionary and (ii) the Allshortcuts dictionary – is there are reason for this duplication? If you don’t know where the short cut is, then there are only two possible choices: it’s either in the truck dictionary or the car dictionary.  You could possibly write a method if you really wanted which searches both the dictionaries and returns the appropriate value.
Since you have access directly to the dictionaries as properties – then why have a AddShortCut and RemoveShortCut method – you can directly access the dictionary via the property. Sure an OOP diehard might insist on you using a getter method – but you still have one location, one point of all when accessing those dictionaries via the property.
Is there any reason to have a static class?

Something like this may be easier.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Text;
using System.Threading.Tasks;

namespace practiceConsole
{///
    internal class Program
    {
        #region public properties and variables

        private Dictionary<string, Type> _truckShortCuts = new Dictionary<string, Type>();

        public Dictionary<string, Type> TruckShortCuts
        {
            get { return _truckShortCuts; }
            set { _truckShortCuts = value; }
        }

        private Dictionary<string, Type> _carShortcuts = new Dictionary<string, Type>();

        public Dictionary<string, Type> CarShortCuts
        {
            get { return _carShortcuts; }
            set { _carShortcuts = value; }
        }

        #endregion public properties and variables

        private static void Main(string[] args)
        {
        }

        public Type GetShortCut(string key)
        {
            Type truckValue;
            if (TruckShortCuts.TryGetValue(key, out truckValue))
            {
                return truckValue;
            }
            else
            {
                // couldn't find the key in trucks.
                // so let's search for it in Cars

                Type carValue;
                if (CarShortCuts.TryGetValue(key, out carValue))
                {
                    return carValue;
                }
                else
                {
                    // couldn't find the key.
                    // throw exception
                    throw new KeyNotFoundException("\nCouldn't find the key in either the car or trucks shortcut dictionaries, my regrets.");
                }
            }
        }
    }
}


Creating smooth animations using rAF It is my first time trying to create smooth animations using rAF and I would like some insight from more experienced developers. Basically I have 16 elements with x,y coordinates and on mousemove over their parent container, all elements move a set amount in the opposite direction. When I do tests on lower-end machines there is some stutter. Is there any technique I am not aware of that can boost my animation performance?

(function ($) {
$(document).ready(() => {
const animCont = document.querySelector('.shop-header-container');
const animContWidth = animCont.offsetWidth;
const animContHeight = animCont.offsetHeight;
const leftOffset = animCont.getBoundingClientRect().left;
const topOffset = document.querySelector('#primary').offsetTop;
const animImgs = animCont.querySelectorAll('.elem');
let mouseX = 0;
let mouseY = 0;
let mouseOver = false;
let positions = [
  {
    id: 82,
    start: {
      x: 0.596,
      y: 0.654,
    },
    range: 0.4,
  }
...
]
positions.forEach(el => {
  el.target = {};
  el.current = {};
})
function updateElements(){
  mouseTargetX = (mouseX - leftOffset) / animContWidth;
  mouseTargetY = (mouseY + topOffset) / animContHeight;
  positions.forEach((el,i) => {
    if (mouseOver) {
      el.target.x = ((mouseTargetX - el.start.x) * -el.range + el.start.x ) * animContWidth;
      el.target.y = ((mouseTargetY - el.start.y) * -el.range + el.start.y ) * animContHeight;
    } else {
      el.target.x = el.start.x * animContWidth;
      el.target.y = el.start.y * animContHeight;
    }

    if(!el.current.x) {
      el.current.x = el.target.x;
      el.current.y = el.target.y;
    } else {
      el.current.x = el.current.x + (el.target.x - el.current.x)*0.1;
      el.current.y = el.current.y + (el.target.y - el.current.y)*0.1;
    }

    animImgs[i].style.transform = 'translate3d('+el.current.x+'px,'+el.current.y+'px,0)';
  })
  requestAnimationFrame(updateElements);
}
updateElements();
function animContMouseEnter(e){
  mouseOver = true;
}
function animContMouseLeave(){
  mouseOver = false;
}
function animContMouseOver(e){
  mouseX = e.clientX;
  mouseY = e.clientY;
}
animCont.addEventListener("mousemove",animContMouseOver,false);
animCont.addEventListener("mouseenter",animContMouseEnter,false);
animCont.addEventListener("mouseleave",animContMouseLeave,false);
});
}(jQuery));

There is not enough information in your question to get a clear picture of where there may be a problem.

Your code

You code does not have any glaring performance issues. Some minor changes would be...

Use for loops rather than forEach. (Minor performance increase but when animating its always good to use the fastest style)
Don't use 3D transforms if you are only doing 2D animation. Better yet, if only 2d dont use the transform ate all. Set thestyle.positiontoabsoluteand use thetop,left` CSS style attributes to set position.
You only use jQuery to get the onload event. Its just bloat so don't load it if you don't need it.

Animating elements.

Some points related to animating elements.

Its not just the elements that you are animating that add to the total load of an animation. Other elements may require rendering (compositing) and they may also be triggering reflow/s. Composites and reflows are expensive operations

The order of the animated elements in relationship to other content can have a big impact on performance due to inefficient compositing. For example you may be animating items A and B, and on the page you have c, A, d, B, e (where c,d,e are other elements) . In the worst case the compositing of the content will be as follows... render c, composite c with background, render A, composite A with previous composite, render b, composite b with previous composite, and so on.

So you just change and move 2 items but that may trigger 5 renders and 5 composites, and depending on the layout, they may be the 5 full screen composites and renders each frame.

Are you using filters? Depending on the filters (and the GPU's abilities) they can add to the workload. Use filters sparingly, don't make more than one change to filters during a frame.

Can all the image and pixel related content fit into GPU RAM. If not reduce the number of pixels. Low end devices can have very low amounts of GPU RAM and if you exceed the capacity of the GPU the browser will swap it in and out of CPU RAM as needed. This is SLOW... very SLOW...

Take charge of the rendering.

It can be very difficult to locate animation bottle necks when animating elements.

One way to remove the rendering/compositing ambiguity is to use the canvas. You then get to efficiently control the rendering compositing process. The canvas 2D and WebGL API's use the same code (there abouts) as the browsers rendering and compositing systems.

With some minor changes your animation items can be rendered on a canvas, needing only one render pass and one composite (if you also render the background you can eliminate composite)

The snippet is an example of canvas rendering performance. There are 500 bugs
each rendered with a full transform scale, position, and rotated. This you would never be able to do using HTML elements.

requestAnimationFrame(update); // Start animation when ready

const image = new Image;
image.src = "https://i.stack.imgur.com/C7qq2.png?s=328&g=1";
const canvas = document.createElement("canvas");
var ctx = canvas.getContext("2d");
canvas.style.position = "absolute";
canvas.style.top = "0px";
canvas.style.left = "0px";
document.body.appendChild(canvas);

const mouse  = {x : 0, y : 0}
document.addEventListener("mousemove", (e) => {
 mouse.x = e.pageX;
 mouse.y = e.pageY;
});

var w,h;
function resize(){
    w = canvas.width = innerWidth;
    h = canvas.height = innerHeight;
}
resize();
addEventListener("resize",resize);
const rand  = (min = 1, max = min + (min = 0)) => Math.random() * (max - min) + min;
const DO = (count,callback) => { while (count--) { callback(count) } }
const sprites = [];
DO(500,()=>{
    sprites.push({
       x : rand(w),
       y : rand(h),
       xr : 0,
       yr : 0, // actual position of sprite
       r : rand(Math.PI * 2),
       scale : rand(0.1,0.25),
       dx : rand(-2,2),
       dy : rand(-2,2),
       dr : rand(-0.1,0.1),
    });
});

// Draws a sprite.
function drawImage(image, spr){
    ctx.setTransform(spr.scale, 0, 0, spr.scale, spr.xr, spr.yr); // sets scales and origin
    ctx.rotate(spr.r);
    ctx.drawImage(image, -image.width / 2, -image.height / 2);
}

// Main animation loop
function update(time){
    var ihM,iwM,dx,dy,spr,i,iw,ih,dist;
    ctx.setTransform(1,0,0,1,0,0);
    ctx.clearRect(0,0,w,h);
    const runHalf = time / 3000;
    if(image.complete){
      iw = image.width;
      ih = image.height;
      const size = (iw * iw + ih * ih) / 2;
      for(i = 0; i < sprites.length; i ++){
          spr = sprites[i];
          dx = mouse.x - spr.xr;
          dy = mouse.y - spr.yr;
          dist = dx * dx + dy * dy;
          if(dist/ spr.scale < size){
              const dir = -Math.atan2(dy,dx) -0.2 + rand(0.4);
              const speed = rand(18);
              spr.dx += Math.cos(dir) * speed;
              spr.dy += Math.sin(dir) * speed;

          }else if(((i + runHalf) | 0) % 2){ // half run away
              spr.dx += dx * 0.001 + Math.cos(spr.r) * 0.1;
              spr.dy += dy * 0.001 + Math.sin(spr.r) * 0.1;
          }else{
              spr.dx += -dx * 0.001 + Math.cos(spr.r) * 0.1;
              spr.dy += -dy * 0.001 + Math.sin(spr.r) * 0.1;
          }

          spr.dx *= 0.99;
          spr.dy *= 0.99;
          // limit speed
          dist = spr.dx * spr.dx + spr.dy * spr.dy;
          if(dist > 54){
              spr.dx *= 0.9;
              spr.dy *= 0.9;
          }

          spr.x += spr.dx;
          spr.y += spr.dy;
          spr.r += spr.dr;
          iwM = iw * spr.scale * 2 + w;
          ihM = ih * spr.scale * 2 + h;
          spr.xr = ((spr.x % iwM) + iwM) % iwM - iw * spr.scale;
          spr.yr = ((spr.y % ihM) + ihM) % ihM - ih * spr.scale;
          drawImage(image,spr);
      }
    }
    requestAnimationFrame(update);
}


Redraw text on change, with automatic zooming, using fabric.js My code works, but I think that my code is non-standard, not optimized, and has poor logic.  I want to improve my coding skills, so please suggest how to improve my code.

var canvas = new fabric.Canvas('editor');
// Fabricjs
var textbox = new fabric.Text('Your Text', {
  left: 100,
  top: 0,
  width: 800,
  textAlign: 'center',
  fontFamily: "Impact",
  fontSize: 76,
  radius: 230,
  rotate: 0,
  selectable: false,
  hasControls: false,
  effect: "STRAIGHT",
  charSpacing: 10,
  spacing: 40
});
canvas.add(textbox).renderAll();
canvas.setActiveObject(canvas.item(canvas.getObjects().length - 1));

// Apply selected Text on change
jQuery("#Eletter").keyup(function() {
  $Eletter = jQuery("#Eletter").val();
  var obj = canvas.getActiveObject();
  //alert(isFloat(obj.getWidth()));
  if (obj) {
    // $priceHight = calcu();
    var widc = obj.getWidth().toFixed(0);
    //console.log(parseInt(widc));
    var letterins = jQuery("#Eletter").val();
    var letterLenghts = letterins.length;
    var LetteringL = jQuery("#LetteringL").val();
    if (letterLenghts > LetteringL) {
      if (widc >= 400.00 && widc <= 430.00) {
        // alert("am working");
        canvas.setZoom(canvas.getZoom() / 1.1);
      } else if (widc >= 600 && widc <= 630.00) {
        canvas.setZoom(canvas.getZoom() / 1.1);
      } else if (widc >= 800 && widc <= 830.00) {
        canvas.setZoom(canvas.getZoom() / 1.2);
      } else if (widc >= 1000 && widc <= 1030.00) {
        canvas.setZoom(canvas.getZoom() / 1.2);
      } else if (widc >= 1200 && widc <= 1230.00) {
        canvas.setZoom(canvas.getZoom() / 1.2);
      } else if (widc >= 1400 && widc <= 1430.00) {
        canvas.setZoom(canvas.getZoom() / 1.1);
      } else if (widc >= 1600 && widc <= 1630.00) {
        canvas.setZoom(canvas.getZoom() / 1.1);
      } else if (widc >= 1800 && widc <= 1830.00) {
        canvas.setZoom(canvas.getZoom() / 1.1);
      } else if (widc >= 1200 && widc <= 1230.00) {
        canvas.setZoom(canvas.getZoom() / 2.1);
      }
    } else {

      if (widc >= 400.00 && widc <= 430.00) {
        //alert("am working");
        canvas.setZoom(canvas.getZoom() * 1.1);
      } else if (widc >= 600 && widc <= 630.00) {
        canvas.setZoom(canvas.getZoom() * 1.1);
      } else if (widc >= 800 && widc <= 830.00) {
        canvas.setZoom(canvas.getZoom() * 1.2);
      } else if (widc >= 1000 && widc <= 1030.00) {
        canvas.setZoom(canvas.getZoom() * 1.2);
      } else if (widc >= 1200 && widc <= 1230.00) {
        canvas.setZoom(canvas.getZoom() * 1.2);
      } else if (widc >= 1400 && widc <= 1430.00) {
        canvas.setZoom(canvas.getZoom() * 1.1);
      } else if (widc >= 1600 && widc <= 1630.00) {
        canvas.setZoom(canvas.getZoom() * 1.1);
      } else if (widc >= 1800 && widc <= 1830.00) {
        canvas.setZoom(canvas.getZoom() * 1.1);
      } else if (widc >= 1200 && widc <= 1230.00) {
        canvas.setZoom(canvas.getZoom() * 2.1);
      }
    }
  }
  //  canvas.setHeight(obj.getHeight() + 200);
  // obj.set("top", 0);
  canvas.getActiveObject().setText($Eletter);

  canvas.renderAll();
  jQuery("#LetteringL").val(letterLenghts);
});
<script src="https://cdnjs.cloudflare.com/ajax/libs/fabric.js/1.7.20/fabric.min.js"></script>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.7.2/jquery.min.js"></script>

<div class="wrapper-canvas">
  <canvas class="" id="editor" width="1000" height="auto"></canvas>
</div>

<input class="text-input" type="text" id="Eletter" value="Your Text">

<input id="LetteringL" type="hidden" value="">

This example is also available as a Fiddle.

Review and rewrite.
Code

Use strict mode by adding the directive "use strict"; to the top of the javascript file.

Use const for variables that do not change.

Don't query the DOM every time you want an element. Get it once, and store it for later use.

Don't duplicate data. You have $Eletter and letterins both of which hold the same data.

toFixed returns a string not a number so be careful with its use. You can use Math.round(value) to round a value.

Don`t create copies of variables if you only use them once.

The following are all not needed
var letterins = jQuery("#Eletter").val();
var letterLenghts = letterins.length;
var LetteringL = jQuery("#LetteringL").val();

Use arrays as lookups when you have sets of data with values attached.

Eg your zoom settings (BTW you repeated the 1200 zoom setting of 2.1 it will never be set)
// a list of the widths and associated zooms
const zoomSettings = [
    {widths: [400, 600, 1400, 1600, 1800], zoom: 1.1},
    {widths: [800, 1000, 1200], zoom: 1.2}
];

// function gets the zoom setting depending on width
// if a matching zoom can not be found then a zoom of 1 is returned
const getZoom = (width) => {
    var zoom = zoomSettings.find(arr => {
       return arr.widths.some(val => width >= val && width <= val + 30)
    });
    if (zoom) { return zoom.zoom }
    return 1;
}

Now you don't have to have the long chain of if else statements. For details on the array methods some and find go to MDN array
jQuery
Do you really need jQuery?
I have never approved of jQuery for many reasons, one of them being that it encourages bad practices. These days its primary reason for existence (cross browser compatibility) no longer applies and it is a large complex script that your clients need to download, parse, and execute, which is a cost that they must bear.
Now I have an even better argument against its use. Using jQuery opens your site to security vulnerabilities see 77% of 433,000 Sites Use Vulnerable JavaScript Libraries
Thus I remove jQuery, as it is not needed and can be done with...
function query(query){
    return document.querySelector(query);
}

This is not a cover all solution but just a short cut to the standard DOM query.
A rewrite
I have rewritten the code using the above as a guide.
I have written it to do the same as your code as I am not sure what your aim is with the zoom setting, they seem arbitrary and limited to only a small set of possible text widths.

"use strict";
const canvas = new fabric.Canvas('editor');
const textbox = new fabric.Text('Your Text', {
  left: 100,
  top: 0,
  width: 800,
  textAlign: 'center',
  fontFamily: "Impact",
  fontSize: 76,
  radius: 230,
  rotate: 0,
  selectable: false,
  hasControls: false,
  effect: "STRAIGHT",
  charSpacing: 10,
  spacing: 40
});
canvas.add(textbox).renderAll();
canvas.setActiveObject(canvas.item(canvas.getObjects().length - 1));

const zoomSettings = [
    {widths: [400, 600, 1400, 1600, 1800], zoom: 1.1},
    {widths: [800, 1000, 1200], zoom: 1.2}
];
const getZoom = (width) => {
    var zoom = zoomSettings.find(arr => {
        return arr.widths.some(val => width >= val && width <= val + 30);
    });
    if (zoom) { return zoom.zoom }
    return 1;
}
function query (query) { return document.querySelector(query) }
const eLetter = query("#Eletter");
const letteringL = query("#LetteringL");
eLetter.addEventListener("keyup", keyUpEvent);
function keyUpEvent(event) {
    const obj = canvas.getActiveObject();
    if (obj) {
        const widc = Math.round(obj.getWidth());
        if (event.target.value.length > letteringL.value) {
            canvas.setZoom(canvas.getZoom() / getZoom(widc));
        } else {
            canvas.setZoom(canvas.getZoom() * getZoom(widc));
        }
    }
    canvas.getActiveObject().setText(event.target.value);
    canvas.renderAll();
    letteringL.value = event.target.value.length;
}
<script src = "https://cdnjs.cloudflare.com/ajax/libs/fabric.js/1.7.20/fabric.min.js" ></script>
<div class="wrapper-canvas">
  <canvas class="" id="editor" width="1000" height="auto"></canvas>
</div>
<input class="text-input" type="text" id="Eletter" value="Your Text">
<input id="LetteringL" type="hidden" value="">


Match values inside object and add them Does the following code is written correct witout redundant code ?
or maybe using Jquery it can be written better?

I need to find matching between properties insdie oData &  oPushObj
and fill aSelData with this content.

This code is working as expected

        jQuery.each(aDataCollection, function(iIndex, oData) {
            oPushedObject = {};

            fnAddEnt(aProperties, oData, oPushObj);

            Object.getOwnPropertyNames(oData).forEach(function(key) {
                if (oPushObj.hasOwnProperty(key)  {

                    var source = oData[key].results[iIndex];
                    var destination = oPushObj[key];

                    Object.getOwnPropertyNames(source).forEach(function(sourceKey) {
                        if (destination.hasOwnProperty(sourceKey)) {
                            var sourceItem = source[sourceKey];
                            destination[sourceKey] = sourceItem;
                        }
                    });
                }
            });

            aSelData.push(oPushObj);
        });
        return aSelData;
    };

General.

I notice that this question is a new expression of the other one you asked previously... but have now deleted!

In the previous question you didn't really explained something that becomes clear with this new one: you're looking for matching properties (with unknown names) successively at two nesting levels. In addition, 1st step works at level 1 of each object, while 2nd step works at level 2 for one object and level 3 for the other.

It's not a good idea to have deleted your previous question because, in the other hand, it showed an example of the involved objects structure. Since I remembered that, I could understand what your current code does (and BTW, understand what was not previously clear).
At the opposite, for somebody reading your current question, and without the knowing of the structure, it's pretty hard to see what your code is intended to.

About the current code.

The part of code you posted is inconsistent:

Its 2 last lines show that it's part of some loop, whose beginning is missing.
Yet at the beginning, it also lacks at least a declaration var aSelData = [];.
The assignation oPushedObject = {}; is out of date: this object is named oPushObj in the rest of the code.

Technically speaking, there are 3 points to notice:

Apart from its erroneous name, the above cited assignation should be var oPushObj = {}; in order not to pollute the global space.
Object.getOwnPropertyNames() is not the best way to get the involved properties, because it returns all properties, both enumerable or not, and with the prototype's one added: merely use Object.keys().
Likewise, if (oPushObj.hasOwnProperty(key) (note it lacks a closing parenthesis) is overkill where if (oPushObj[key]) is enough.

Note (thanks to @Roamer-1888 for pointing it): this last suggestion is pertinent only if the oPushObh[key] can't contain any falsy value.

Taking in account all the remarks above, here is a modified version of your code, where I also added some comments to explain how it works:

var aSelData = [];
jQuery.each(aDataCollection, function(iIndex, oData) {
    var oPushObj = {};
    // Here oPushObj gets populated
    fnAddEnt(aProperties, oData, oPushObj);
    // Look for matching properties between oPushObj and oData (1st level)
    Object.keys(oData).forEach(function(key) {
        if (oPushObj[key]) {
            // Look for matching properties between oPushObj[key] (2nd level)
            // and oData[key][iIndex] (3rd level)
            var source = oData[key].results[iIndex];
            var destination = oPushObj[key];
            Object.keys(source).forEach(function(sourceKey) {
                if (destination.[sourceKey]) {
                    // Populate oPushObj[key][sourceKey] value
                    destination[sourceKey] = source[sourceKey];
                }
            });
        }
    });
    aSelData.push(oPushObj);
});

Suggested alternate way

You currently use the simple approach of iterating through objects, for each level of the search process. And sure this works.

But you might also consider a different approach, based on extracting the intersection of the objects properties, successively for the 2 involved levels.
IMO the code is slightly more readable this way, making its main part look as a single-line one.
In the other hand, I couldn't assert it's faster.

Here is it:

var aSelData = [];
jQuery.each(aDataCollection, function(iIndex, oData) {
    var oPushObj = {};
    // Here oPushObj gets populated
    fnAddEnt(aProperties, oData, oPushObj);
    // Look for matching properties between oPushObj and oData (1st level)
    for (var key of arrayIntersect(
      Object.keys(oPushObj), Object.keys(oData)
    )) {
        // Look for matching properties between oPushObj[key] (2nd level)
        // and oData[key][iIndex] (3rd level)
        for (var sourceKey of arrayIntersect(
          Object.keys(oPushObj[key]), Object.keys(oData[key][iIndex])
        )) {
            // Populate oPushObj[key][sourceKey] value
            oPushObj[key][sourceKey] = oData[key][iIndex][sourceKey];
        }
    }
    aSelData.push(oPushObj);
});

function arrayIntersect(a, b) {
  return a.filter(key => b.indexOf(key) > -1)
}


Recursive Ruby merge sort I have implemented a merge sort in Ruby. (Yes, I know, there's Array#sort) I would like to get feedback on code quality and optimization.

def merge(a, b)
  result = []

  while !a.empty? && !b.empty?
    if a[0] <= b[0]
      result << a.shift
    else
      result << b.shift
    end
  end

  result += a += b

  return result
end

def merge_sort(m)
  if m.length <= 1
    return m
  end

  left, right = [], []
  m.each_with_index do |x, i|
    if i < m.length / 2
      left << x
    else
      right << x
    end
  end
  left = merge_sort(left)
  right = merge_sort(right)

  return merge(left, right)
end

input = [*1..100].shuffle

p input

p merge_sort input

You can define left and right in merge_sort this way:

n = m.length
if n <= 1
  return m
end

left = merge_sort(m[0 ... n/2])
right = merge_sort(m[n/2 .. -1])

Depending on your Ruby version, Array#shift can be O(n). You could keep 2 indices for a and b:

def merge(a, b)
  result = []
  i, j = 0, 0
  na, nb = a.size, b.size

  while i < na && j < nb do
    if a[i] <= b[j]
      result << a[i]
      i += 1
    else
      result << b[j]
      j += 1
    end
  end

  return result + a[i..-1] + b[j..-1]
end

With those changes, the code becomes twice as fast with input = [*1..100000].shuffle.


Checking when capslock is turned on I've written this JQuery program for checking whenever caps lock is turned on.

$("input[type='password']").keypress(function(e) {
    var kc = e.which; //get keycode
    var isUp = (kc >= 65 && kc <= 90) ? true : false; // uppercase
    var isLow = (kc >= 97 && kc <= 122) ? true : false; // lowercase
    // event.shiftKey does not seem to be normalized by jQuery(?) for IE8-
    var isShift = ( e.shiftKey ) ? e.shiftKey : ( (kc == 16) ? true : false ); // shift is pressed

    // uppercase w/out shift or lowercase with shift == caps lock
    if ( (isUp && !isShift) || (isLow && isShift) ) {
        capLock(); // alerts "CAPSLOCK is ON"
    }

});
function capLock() {
 alert('CAPSLOCK is ON');
}

Turning my comment on your question into an answer:

On a Mac, when the Caps Lock key is engaged, the Shift key doesn't change the case to lower. So in the event that the Caps Lock key has already been engaged, before your page even loads, this code won't work as intended. I believe this jQuery plugin has already solved this problem - github.com/nosilleg/capslockstate-jquery-plugin

As for your code itself:

I like that you've got named variables, that makes the final if statement super easy to read. You could name the event variable as such too, so it's clearer what e contains.
You can refactor this var isUp = (kc >= 65 && kc <= 90) ? true : false; // uppercase

to:

var isUp = (kc >= 65 && kc <= 90) // uppercase

because (kc >= 65 && kc <= 90) evaluates to a boolean value. The conditional operator is redundant.

Similarly, you can refactor: var isShift = ( e.shiftKey ) ? e.shiftKey : ( (kc == 16) ? true : false );

to:

var isShift = ( e.shiftKey ) || (kc == 16);

The interesting thing about the statement above is that it takes advantage of a mechanism called short-circuit evaluation that boolean operators, like OR (||) and AND (&&), use.

Basically, in an OR statement, if the first expression evaluates to true, the second expression is never evaluated, because true || anything will always evaluate to true.

Similarly for an AND statement, if the first expression evaluates to false, the second expression is never evaluated, because false && anything will always evaluate to false.


Creating linked list data with iterator pattern I've created a linked list to use it in SquareList.  I've started to create a linked list based on just that.  I want my class to follow the iterator pattern.  How can it be improve to use vector iterator?

#ifndef LINKEDLIST_HPP
#define LINKEDLIST_HPP

#include <stdlib.h>

template <typename Object>
class Linkedlist
{
  private:
    // The basic doubly linked Linkedlist node.
    // Nested inside of Linkedlist, can be public
    // because the Node is itself private
    struct Node
    {
        Object  _data;
        Node   *_prev;
        Node   *_next;

        Node( const Object & d = Object( ), Node * p = NULL, Node * n = NULL )
          : _data( d ), _prev( p ), _next( n ) { }
    };

  public:

  public:
    Linkedlist( )
      { init( ); }

    ~Linkedlist( )
    {
        clear( );
        delete _head;
        delete _tail;
    }

    Linkedlist( const Linkedlist & rhs )
    {
        init( );
        *this = rhs;
    }

    const Linkedlist & operator= ( const Linkedlist & rhs )
    {
        if( this == &rhs )
            return *this;
        clear( );
        for( const_iterator itr = rhs.begin( ); itr != rhs.end( ); ++itr )
            push_back( *itr );
        return *this;
    }

    // Return iterator representing beginning of Linkedlist.
    // Mutator version is first, then accessor version.
    iterator begin( )
      { return iterator( _head->_next ); }

    const_iterator begin( ) const
      { return const_iterator( _head->_next ); }

    // Return iterator representing endmarker of Linkedlist.
    // Mutator version is first, then accessor version.
    iterator end( )
      { return iterator( _tail ); }

    const_iterator end( ) const
      { return const_iterator( _tail ); }

    // Return number of elements currently in the Linkedlist.
    int size( ) const
      { return _size; }

    // Return true if the Linkedlist is empty, false otherwise.
    bool empty( ) const
      { return size( ) == 0; }

    void clear( )
    {
        while( !empty( ) )
            pop_front( );
    }

    // front, back, push_front, push_back, pop_front, and pop_back
    // are the basic double-ended queue operations.
    Object & front( )
      { return *begin( ); }

    const Object & front( ) const
      { return *begin( ); }

    Object & back( )
      { return *--end( ); }

    const Object & back( ) const
      { return *--end( ); }

    void push_front( const Object & x )
      { insert( begin( ), x ); }

    void push_back( const Object & x )
      { insert( end( ), x ); }

    void pop_front( )
      { erase( begin( ) ); }

    void pop_back( )
      { erase( --end( ) ); }

    // Insert x before itr.
    iterator insert( iterator itr, const Object & x )
    {
        Node *p = itr.current;
        _size++;
        return iterator( p->_prev = p->_prev->_next = new Node( x, p->_prev, p ) );
    }

    // Erase item at itr.
    iterator erase( iterator itr )
    {
        Node *p = itr.current;
        iterator retVal( p->_next );
        p->_prev->_next = p->_next;
        p->_next->_prev = p->_prev;
        delete p;
        _size--;

        return retVal;
    }

    iterator erase( iterator start, iterator end )
    {
        for( iterator itr = start; itr != end; )
            itr = erase( itr );

        return end;
    }

  private:
    int   _size;
    Node *_head;
    Node *_tail;

    void init( )
    {
        _size = 0;
        _head = new Node;
        _tail = new Node;
        _head->_next = _tail;
        _tail->_prev = _head;
    }
};

#endif

You define the copy constructor in terms of the assignment operator

Linkedlist( const Linkedlist & rhs )
{
    init( );
    *this = rhs;
}

It is more traditional to do it the other way around. Normally you define the assignment in terms of the copy constructor. The idiom is called "Copy and Swap"

Linkedlist(Linkedlist const& rhs)
{
    // Create a copy
}

Linkedlist& operator=(Linkedlist rhs) // pass by value to get copy
{
    rhs.swap(*this);   // swap the copy with this.
    return *this;      // As rhs leaves scope with the old values
}                      // it is automatically deleted.

There are a couple of problems with doing it the other way around.

You do not provide the strong exception guarantee. On an assignment you destroy the content of you list. Then start to populate it with new values. If at any point copying fails you are left with a half complete object and no way to roll back to the original object.

Assignment should fall into three distinct phases.

A = B;

Create a copy of the object being assigned from B into a temporary object. This will be the new value that A holds once everything works. If this copy fails then the value of A should not be changed.
Swap the internal state of A with the temporary object. Since a swap should never fail this has no danger. A will now hold a copy of B and the temporary object will hold the old value of A.
Destroy the old object. If this fails it does not matter. As the state of A is now consistent.

const Linkedlist & operator= ( const Linkedlist & rhs )
{
    if( this == &rhs )
        return *this;
    clear( );

At this point A has been cleared. Thus if the folloping loop fails you have no way to recover the original value of A. This will leave your program in a funny state. You have provided The Basic Exception Guarantee in that the object is not invalid. But this is not usually acceptable.

    for( const_iterator itr = rhs.begin( ); itr != rhs.end( ); ++itr )
        push_back( *itr );

    return *this;
}

This seems a bit over-elaborate!

Object & front( )
  { return *begin( ); }

You are creating an iterator just to de-reference it! Why not just

Object & front( )
  { return head->next.data; }

You should also put a comment that it is undefined behavior when the list is empty. Best to document these things up front.x

Not to sure you need to define two different classes for iterator and const_iterator. Seems like a lot of duplicated code.

Also you iterator does not meet even the requirements to trivial iterator as you are missing operator ->. See http://www.sgi.com/tech/stl/table_of_contents.html specifically look at the iterator section. I presume you are actually trying to implement the concept of Bi-Directional iterator the definition can be found here http://www.sgi.com/tech/stl/BidirectionalIterator.html

In addition your container object is supposed to provide some specific types that allow the user of your class to obtain information about the content type. Requirements can be found here: http://www.sgi.com/tech/stl/Container.html

See here for an example of an iterator:   https://codereview.stackexchange.com/a/9399/507 (though I did not make the container as well as I could because I did not specify the types I should have).

See here for an example of a linked list using a single sentinel.
https://codereview.stackexchange.com/a/126007/507


MergeSort w/ Custom Functions Please review my implementation of mergesort:

mergesort' :: (Ord a) => [a] -> [a]
mergesort' []     = []
mergesort' xs = merge' (sort' fst', sort' snd')
   where fst' = take half xs
         snd' = drop half xs
         half = len `div` 2
         len = length xs

merge' :: (Ord a) => ([a], [a]) -> [a]
merge' ([], [])      = []
merge' ([], ys)      = ys
merge' (xs, [])      = xs
merge' (x:xs, y:ys) = if x < y then x : merge' (xs, y:ys) else y : merge'(x:xs, ys)

sort' :: (Ord a) => [a] -> [a]
sort' []     = []
sort' (x:xs) = m : sort' rest
  where m = foldl (\acc x -> if x < acc then x else acc) x xs
        rest = filterOne' (/= m) (x:xs)

filterOne' :: (a -> Bool) -> [a] -> [a]
filterOne' _ []     = []
filterOne' f (x:xs) = if not $ f x then xs else x : filterOne' f xs

If it doesn't adhere to the mergesort algorithm, please let me know.

Also, is there a Haskell library function for my filterOne'?

This isn't mergesort! sort' is a different sorting algorithm entirely, so you end up only performing one merge step before kicking it over to (I believe) selection sort.

First let's address the easy stylistic issues so we have a good base to work from. The primary issue is that the argument passed to merge' is a tuple, which is unnecessary, uncurried, and not very Haskell-y. I'll also drop the extra apostrophes where there isn't an actual naming conflict with the Prelude. We use as-patterns to avoid reconstructing patterns we just deconstructed. And let's just toss sort' and filterOne', they're not part of the solution!

mergesort :: (Ord a) => [a] -> [a]
mergesort []     = []
mergesort xs     = merge (??? left) (??? right)
    where half = (length xs) `div` 2
          left  = take half xs
          right = drop half xs

merge :: (Ord a) => [a] -> [a] -> [a]
merge [] []                 = []
merge xs []                 = xs
merge [] ys                 = ys
merge xss@(x:xs) yss@(y:ys) = if x <= y then x : merge xs yss else y : merge xss ys
                                 -- ^ Less than *or equal* so our sort is stable

One question is what goes in for the ??? where we used to call sort'? Well, mergesort!

One more complex issue of style is the usage of if in merge. Top-level ifs are better represented as guards in Haskell, it's easier to extend guards and their usage is more idiomatic.

merge xss@(x:xs) yss@(y:ys) | x <= y    = x : merge xs yss
                            | otherwise = y : merge xss ys

One small change we can make is adding another base case to mergesort to account for lists of length 1, which are by definition sorted.

mergesort xs@[x] = xs

There's only one more improvement I'd make to this, and that's to call splitAt rather than take and drop in mergesort. Using both take and drop leads to walking half the list twice in each recursive step where we can walk it only once by doing some more bookkeeping. splitAt takes an index to split a list at, and returns the prefix and remainder just like using take and drop separately would.

So putting all of that together, here's the final version.

mergesort :: (Ord a) => [a] -> [a]
mergesort []     = []
mergesort xs@[x] = xs
mergesort xs     = merge (mergesort left) (mergesort right)
    where half = (length xs) `div` 2
          (left, right) = splitAt half xs

merge :: (Ord a) => [a] -> [a] -> [a]
merge [] []                 = []
merge xs []                 = xs
merge [] ys                 = ys
merge xss@(x:xs) yss@(y:ys) | x <= y    = x : merge xs yss
                            | otherwise = y : merge xss ys


Expanding powers of expressions of the form ax+b I solved the following problem:

  Write a function expand that takes in an expression with a single, one character variable, and expands it. The expression is in the form (ax+b)^n where a and b are integers which may be positive or negative, x is any one-character long variable, and n is a natural number. If a = 1, no coefficient will be placed in front of the variable. If a = -1, a "-" will be placed in front of the variable.
  The expanded form should be returned as a string in the form ax^b+cx^d+ex^f... where a, c, and e are the coefficients of the term, x is the original one character variable that was passed in the original expression and b, d, and f, are the powers that x is being raised to in each term and are in decreasing order. If the coefficient of a term is zero, the term should not be included. If the coefficient of a term is one, the coefficient should not be included. If the coefficient of a term is -1, only the "-" should be included. If the power of the term is 0, only the coefficient should be included. If the power of the term is 1, the caret and power should be excluded.

Examples:

expand("(p-1)^3");      // returns "p^3-3p^2+3p-1"
expand("(2f+4)^6");     // returns "64f^6+768f^5+3840f^4+10240f^3+15360f^2+12288f+4096"
expand("(-2a-4)^0");    // returns "1"
expand("(-12t+43)^2");  // returns "144t^2-1032t+1849"
expand("(r+0)^203");    // returns "r^203"
expand("(-x-1)^2");     // returns "x^2+2x+1"

My solution passed all tests but I would like to know how it can be improved.

function expand(str) {
    const fac = n => n < 2 ? 1 : n * fac(n - 1);
    let result = '', [_, a, x, b, n] = str.match(/\((-?\d*)([a-z])([-+]\d+)\)\^(\d+)/);

    a = a ? a == '-' ? -1 : parseInt(a) : 1;
    b = parseInt(b); n = parseInt(n);

    for (let i = n; i >= 0; i--) {
        let k = n - i;
        let c = !b && k > 0
            ? 0
            : a**i * b**k * (k == 0
                ? 1
                : fac(n) / (fac(k) * fac(n - k)));

        if (Math.abs(c) == 1 && i > 0) c = c > 0 ? '+' : '-';
        else c = c > 0 ? `+${c}` : c;
        if (c) result += c;

        if (i > 0 && c) result += x;
        if (i > 1 && c) result += `^${i}`;
    }

    return result[0] == '+' ? result.substr(1) : result;
}

I got this problem from a coding challenges site. https://www.codewars.com/kata/binomial-expansion All the context that i had to solve it came from the description of the task.

I did not try to obfuscate the code. As Roland Illig stated, most of the single-letter variables come from the task. Variables a, x, b, n come from the parts of the input expression of the form (ax+b)^n. Variable k comes from a math formula n!/(k!(n-k)!) for the binomial coefficient. Variable c is the coefficient of the term being formed, and its mentioned in ax^b+cx^d+ex^f. So names come from problem's domain.

I mostly just do coding challenges for fun, and I like to golf things a bit. I concede the code lacks readability though.

My main concern is that small numbers like 203 have large factorials and produce overflow. One of the test cases is expand("(r+0)^203");    // returns "r^203". I did not wanted to treat this case separately. Hence I used conditions !b && k > 0 and k == 0 to avoid calculating factorials for "large" n when b == 0. I would like to change this with a better approach for calculating the binomial coefficients. Sadly I don't know enough math.

If someone solves this problem differently I would love to see their code.

I don't have too much experience writing questions in stackexchange and English is not my native language. Tried to format the text to the best of my ability. Thanks all for your patience.

You don't need the special case for k == 0. When k == 0, the result of the other expression will be 1 as well.

For this sequence of binomial coefficients you don't need to calculate the full fak expression each time. You can also start with c = a ** i, and then, for each k, multiply by (n - k) / (k + 1) * b / a. That's a bit faster and provides less risk of producing numeric overflows, which would result in slightly incorrect coefficients. The Wikipedia article on binomial coefficients should explain this in more detail.


Conditionally inserting MySQL entries in a Flask app I am a newbie in back-end developing. I know that the biggest part of back-end development is interaction with database.

I established a connection and the code is 100% working, but I am wondering if it is well optimized. There is one function where I make double insertion into the database but before I commit this insertion I make two requests to the database to determine whether I should insert or not.

from flask import Flask, jsonify, request, url_for
import MySQLdb

def connection():
    conn = MySQLdb.connect(host='somehost',
                           user='root',
                           passwd='some password',
                           db='users_addresses',
                           port=3306
                           )
    c = conn.cursor()
    return c, conn

def processing():
    # some actions
    try:
        c, conn = connection()
        c = conn.cursor()
        mc_address_existence, id = mc_address_exist(c, conn, mc_address=box_id)
        if mc_address_existence and not login_exist(c, conn, login):

            c.execute("INSERT INTO accounts_info (uid, first_name, second_name, email, phone_number) "
                      "VALUES ({},"
                            " '{}',"
                            " '{}', "
                            "'{}',"
                            " {} ) ".format(id, first_name, second_name, email, phone_number)
                      )

            c.execute("INSERT INTO logins_passwords (uid, login, password) "
                      "VALUES ({},"
                      "'{}',"
                      " '{}' ) ".format(id, login, password)
                      )
            conn.commit()
            c.close()
            conn.close()
            return jsonify({'result': True})
        return jsonify({'result': False})
    except Exception as e:
        return str(e)

And here are two functions for determining whether to make the insertion or not:

def mc_address_exist(c, conn, mc_address):
    # Should I wrap it for try/except?
    # c = conn.cursor()
    c.execute("SELECT uid, mc_address "
              "FROM boxes_id "
              "WHERE mc_address='{}' ".format(mc_address))
    res = c.fetchall()
    # c.close() Should I close?
    # conn.close()
    if res:
        return True, res[0][0]
    return False, 0

def login_exist(c, conn, login):
    # should I wrap it for try/except?
    c.execute("SELECT * "
              "FROM logins_passwords "
              "WHERE login='{}' ".format(login))
    res = c.fetchall()
    # c.close() should I close it or not?
    # conn.close()
    if res:
        return True
    return False

It is working code, but maybe not optimized. Here are three questions:

Should I close the cursor (conn, c) in the checking functions?
Should I wrap the database's selection checking functions into try/except?
Maybe I should use with as instead?

def connection():

This is misnamed. Please call it cursor_connection(), as that is the tuple it returns. Some folks would prefer the identifier curs over the rather ambiguous c, as "cursor" and "connection" both start with that letter.

    c, conn = connection()
    c = conn.cursor()

Ok, that 2nd line is just bizarre. Please delete it, as the function already accomplished that task.

Nice little INSERT you have there. Be a shame if some punctuation crept into it:  https://www.xkcd.com/327/

except Exception as e:

What is your concern, here? Might be better to elide the try / catch altogether, since there isn't really any "error recovery code" here.

Rather than having mc_address_exist() return a tuple, it would be more natural to return a (potentially empty) list, or return a scalar that potentially is None (assuming the uid column is declared NOT NULL). It's not clear why you asked the DB to retrieve mc_address, since you just discard it.

In login_exist(), rather than asking for *, it would be a cheaper query to just select count(*).

You're not leaking anything with that cursor, so don't worry about closing it. You are correctly closing the underlying TCP connection. Your try/catch handling appears to be adequate for your needs at present, so no change needed.