Node.js password salting/hashing In view of the recent LinkedIn breach, would anyone care to review my data access routines relating to user access?

This is a standard node.js module, accessing a Postgres database.

(function () {

    'use strict';

    var crypto = require('crypto'),
        hash = function (pass, salt) {
            var h = crypto.createHash('sha512');

            h.update(pass);
            h.update(salt);

            return h.digest('base64');
        };

    module.exports.getUser = function (conn, email, password, callback) {
        conn.query({
            name: 'getUser',
            text: 'SELECT "id", "passwordHash" FROM "user" WHERE "email" = $1 LIMIT 1',
            values: [email]
        }, function (err, result) {
            if (err) {
                throw err;
            }
            if (result.rows.length === 0) {
                callback(null);
            } else {
                var newHash = hash(password, email),
                    row = result.rows[0];

                if (row.passwordHash === newHash) {
                    callback({id: row.id, email: email});
                } else {
                    callback(null);
                }
            }
        });
    };

    module.exports.addUser = function (conn, email, password, callback) {
        var newHash = hash(password, email);

        conn.query({
            name: 'addUser',
            text: 'INSERT INTO "user" ("email", "passwordHash") VALUES ($1, $2) RETURNING "id"',
            values: [email, newHash]
        }, function (err, result) {
            var id;

            if (err) {
                console.error(err);
                callback(null);
            } else {
                id = result.rows[0].id;

                callback({id: id, email: email});
            }
        });
    };

}());

I do not see anything immediately wrong with your addUser() and getUser() implementations.
Your hash() implementation could be improved in two areas though.

Salt
As already indicated by Bill Barry in the comments, your should use a better salt.
Some rules to keep in mind when handling salts,

Salts should be unique per password.
 Every password should have a unique salt. Sharing a salt reduces its effectiveness.
Salts should be unpredictable
You should generate a salt using a Cryptographically Secure Pseudo-Random Number Generator (CSPRNG).

You may want to try using crypto.randomBytes(size, [callback]) if available.
Salts should be large
 PKCS#5 recommends a salt be no smaller than 64-bits (8 bytes).
I like to start with 128-bit salts (16 bytes), favoring higher salt-lengths for more passwords and better security.
Salts are not secret
Although purposefully disclosing the salt isn't a good idea, you can store them in cleartext.

Key stretching / Iterations
Wikipedia has a good description on Key Stretching,

  key stretching refers to techniques used to make a possibly weak key, typically a password or passphrase, more secure against a brute force attack by increasing the time it takes to test each possible key.

The crypto module does not appear to offer key stretching natively for sha512.

You should look into using crypto.pbkdf2(password, salt, iterations, keylen, callback) which uses HMAC-SHA1. You shouldn't use SHA1 on its own, but pbkdf2 using HMAC-SHA1 is still secure.
See Password-Based Key Derivation Function 2 for more information.

You will want to set the number of iterations as high as possible while still maintaining an acceptable responsiveness for your application. Though PKCS#5 recommends a minimum of 1000 iterations, 10000+ iterations is fairly typical; computers are only getting faster.


Opinions/Improvements on a run periodically/timer function in Python 2.7 I'm looking for some opinions on this bit of code that I wrote and if there are any ways it can be improved.

The scripts aim is to run the function runme() every 60 seconds with a random interval anywhere between -20 seconds to +40 seconds so it could run randomly anywhere between 40 seconds and 100 seconds.

import sched, time
import random

s = sched.scheduler(time.time, time.sleep)

def periodically(runtime, intsmall, intlarge, function):
    runtime += random.randrange(intsmall, intlarge)
    s.enter(runtime, 1, function, ())
    s.run()

def runme():
    print "hi"

while True:
    periodically(60, -20, +40, runme)

I don't think you do need the scheduler for that, at least not for how you use it.
Your loop schedules 1 event and just afterwards waits until it's finished, since run() waits until the scheduled event(s) are completed.

You can achieve the same by just sleeping for runtime seconds and calling runme() afterwards (you have to sleep before, since the scheduler delays the action for runtime seconds).

Also note that random.randrange(intsmall, intlarge) will return a value between intsmall and intlarge - 1, so your random time will be between 40 seconds and 99 seconds.


PHP - check if array includes value and return value if true I developed the function array_includes which checks if an array contains a value and returns it. I would like to know if there is already such a function which is more efficient or how I can make the function more efficient.

<?php
$params = "colorgroup-b;test;abc";
$paramsArr = explode(";", $params);

var_dump(array_includes("colorgroup", $paramsArr));  //outputs: colorgroup-b

function array_includes($needle, $arr)
{
    foreach($arr as $param) {
        if (strpos($param, $needle) !== false) {
            return $param;
        }
    }
    return false;
}

Your function checks if an array contains a string that contains your given $needle. No, there's no such function in PHP yet. The in_array() function does something else.

You could make your function more efficient if you include the explode() inside the function, like this:

function find_parameters($needle, $parameterStr)
{
    if (strpos($parameterStr, $needle) !== false) {
        $result = [];
        foreach(explode(";", $parameterStr) as $parameter) {
            if (strpos($parameter, $needle) !== false) {
                $result[] = $parameter;
            }
        }
        return $result;
    }
    return false;
}

This function first checks whether the parameter string actually does contain the needle. It can quickly return false when it doesn't. If the needle is present it does what your code does, with the exception that it returns an array. This is done so that it can return multiple results. Suppose your parameter string looks like this:

$params = "colorgroup-a;colorgroup-b;test;abc";

and you test this:

var_dump(find_parameters("colorgroup", $paramsArr));

Then it will not return only colorgroup-a, like your function, but it will return an array with colorgroup-a and colorgroup-b.

So this new function is more efficient, when there are no matching parameters, and it is more correct in that it finds all the parameters.

Yes, it is longer, but shortness of code should not be the main aim when writing code. The use of regular expressions is also very popular for these kind of tasks, and it would result in less code. But the engine behind regular expressions is complex, and would make the code a lot less efficient.


Youtube OAuth security I am working with Youtube OAuth for the first time and I am not that great when it comes to security. I got the following code and would like to hear if you guys can find any security issue:

    $code = $_GET['code'];

    $url = 'https://accounts.google.com/o/oauth2/token';

    $params = array(
        "code" => $code,
        "client_id" => "XXX",
        "client_secret" => "YYY",
        "redirect_uri" => "URL",
        "grant_type" => "authorization_code"
    );

    $curl = curl_init($url);

    curl_setopt($curl, CURLOPT_POST, true);
    curl_setopt($curl, CURLOPT_POSTFIELDS, $params);
    curl_setopt($curl, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
    curl_setopt($curl, CURLOPT_SSL_VERIFYPEER, false);
    curl_setopt($curl, CURLOPT_RETURNTRANSFER, 1);

    $json_response = curl_exec($curl);
    curl_close($curl);
    $authObj = json_decode($json_response);

    $accountInfo = get_data('https://www.googleapis.com/youtube/v3/channels?part=id&mine=true&access_token='.$authObj->access_token.'');

    $accountInfo = json_decode($accountInfo, true);

    $linkedAccount = $db->QueryFetchArrayAll("SELECT * FROM `youtube_accounts` WHERE `channel_id`='".$accountInfo['items'][0]['id']."'");
    $accountExists = $db->QueryFetchArrayAll("SELECT * FROM `youtube_accounts` WHERE `user_id`='".$data['id']."' AND `status` != 3");

Security

Curl Set POST

I don't think that there is a problem with passing user input to curl_setopt as post field (something like http header splitting is not possible, and it doesn't even seem possibly to add additional post fields, or to overwrite existing ones).

Important information via GET

It's not a good idea to send anything confidential via GET. GET requests can be cached, they can be stored in the browser history, they show up in server logs, and so on. Use POST instead.

SQL injection

$accountInfo isn't controlled by you, so you shouldn't trust it, the same probably goes for $data. Never put variable data directly into an SQL query, use prepared statements instead.

CURLOPT_SSL_VERIFYPEER

If you disable this setting, you weaken the security of SSL. Yes, you still encrypt even if CURLOPT_SSL_VERIFYPEER is false, but you would also accept the key of a man in the middle instead of the key from the server, so an attacker could still read the communication.

Here is the warning from the documentation:

  WARNING: disabling verification of the certificate allows bad guys to
  man-in-the-middle the communication without you knowing it. Disabling
  verification makes the communication insecure. Just having encryption
  on a transfer is not enough as you cannot be sure that you are
  communicating with the correct end-point.


Code which pulls from SQL and dumps in Excel I have a working code that pulls data from SQL and dumps in a column. Unfortunately it is extremely slow as it takes around half a second to pull each data point and it needs to be much faster. Are there any ways I can make this more efficient? Is there a different way to do this rather than go row by row?

Sub PopulatePreviousPrice()
Dim mydata As New SQLConnection
Dim sql As String

Call Input_SQLAdd_and_DatabaseName
Dim Duplicate As Long
Duplicate = -2147217900
Dim NoPrice As Integer
NoPrice = 3021
#Error code for SQL error
On Error GoTo Errorhandler

Dim tbl As ListObject
Dim PriceDate As Date
Dim PriceDateText As String
Dim sqlVal
Dim j As Integer

Set tbl = ActiveSheet.ListObjects("DailyTable")

j = 1

PriceDate = tbl.DataBodyRange(1, 25)
PriceDateText = CStr(Format(PriceDate, "dd/mmm/yyyy hh:mm:ss"))

For j = 1917 To 1 Step -1

PriceDate = tbl.DataBodyRange(j, 25)
PriceDateText = CStr(Format(PriceDate, "dd/mmm/yyyy hh:mm:ss"))

sql = "SELECT ClosingPrice " & _
  " FROM ZIPTrading.instrument.DailyPriceBondPref" & _
  " WHERE PriceDate = (SELECT max(PriceDate) From ZIPTrading.instrument.DailyPriceBondPref WHERE PriceDate < '" & PriceDateText & "' AND FIGI = '" & tbl.DataBodyRange(j, 1) & "')" & _
  " AND FIGI = '" & tbl.DataBodyRange(j, 1) & "';"

sqlVal = mydata.SelectResult(sql)

    If IsNull(sqlVal(0, 0)) = False Then

        tbl.DataBodyRange(j, 20) = sqlVal(0, 0)

    Else

    End If
sqlVal(0, 0) = ""

Next j

MsgBox ("All previous prices in table")

Errorhandler:
                If Err = Duplicate Then
                MsgBox ("Error! Data for this date already exists in the data base or data entry error. Check date and BBG pulled values")
                ElseIf Err = NoPrice Then
                    Resume Next
                    'Some securities have no price data at all and this ends the macro without warning. It's dealt with here
                Else
                msg = "Error # " & Str(Err.Number) & " was generated by " _
                & Err.Source & Chr(13) & "Error Line: " & Erl & Chr(13) & Err.Description
                MsgBox msg, , "Error", Err.HelpFile, Err.HelpContext

                End If

End Sub

First of all let me make an important comment not related to performance:

Security issues

I hope your table is only accessible by by a limited number of trusted people. I say this because building a SQL string like this, without further verification, is generally a bad idea. Just do a short online search for SQL injection attack to understand what I mean. Or see bobby-tables.com for a quick reference.

Performance

Now, regarding the performance. The general problem seems to be that you are querying the database for each row separately. That is usually not how you should work with a database. You can pull in larger result sets using an ADODB.Recordset or other means. Unfortunately, we cannot really see how you pull in the data since you do not provide information about mydata. In particular, we do not know what sqlVal is as you declared it (implicitly) as variant. Accordingly, I can only say something on general terms.

If the table in the database you are pulling the data from is not too large, you could probably pull in all data and filter in memory, which is faster.

If the database table is indeed very large, which I presume, you might want to dynamically generate a filtering condition based on the FIGI and dates, which I assume to be able to differ, in your table. (If the dates are always the same, this would allow a simplification.) The goal is to pull all relevant data in one go.

How to do this best really depends on the database you are talking to. If it supports CTEs (common table expressions) and has syntax to create tables on hard-coded values, it is probably best to generate such a table out of your FIGI/date pairs and do a join to build up the condition. Otherwise, you can just use a generated OR (FIGI = xxx AND PriceDate < yyy) chain in the WHERE clause. However, based on your probaly 1917 hard-coded rows, the performance might not be too good. Either filter would be applied in the subquery which would require a new GROUP BY clause on the FIGI. Then, you just join the result to the table, to get your desired values.

If your database provider supports aggregate window functions you can do the filtering on the max PriceDate using those. Then, no subquery and only an IN condition on the FIGI is required.

Without knowing more about the database you talk to, I cannot say any more regarding the query structure, since basically all databases use different SQL dialects.

That said, I wonder why you do not just use an actual database tool, e.g. Access, to query the main database and then just dump the result into the Excel file. (You can insert entire ADODB.Recordsets and you can read in data from an Excel sheet using SQL via ADO, as well.)

General Remarks

Since this is Code Review, I will also provide some thoughts about your code.

First of all, the readability really suffers from the superfluous vertical whitespace, which has been noted in a comment already, and the partial lack of indentation. To fix the indentation, you might want to visit the online VBA indenter. (Full disclosure - I am involved in the project this website belongs to.)

Second, the three executable lines in front of the For loop, do not do anything since the values are overwritten immediately in the loop, which has hard-coded bounds.

Instead of hardcoding the bounds as magic numbers, you might want to consider introducing a constant with a meaningful name.

Another thing I noticed is that you do not exit the the procedure before running into the error handler, i.e.: the error handler gets executed every time you execute the procedure. You should put an Exit Sub in front of it.

Now, a few nit-picky things:

The Call statement you use at the top, is deprecated. It would be more idiomatic to not use it.
You should always specify the accessibility of a procedure. If you really want it to be public, you should state that.
Declaring a variable As New is not the same as declaring it and then setting it to a new instance. If you declare a variable As New it will only be instantiated the first time it is used and it will be re-instantiated on the next use after setting it to Nothing. Since the execution engine has to check each time whether the object still exists, there is an overhead involved.
The use of Integer in favour of Long is largely useless. Internally, the Integer is saved as a Long. See this SO answer.
It is generally better practice to declare variables as close to first usage as possible, contrary to what Microsoft previously suggested. (They have reverted their stand on this in more recent style guides.)
You could separate the different concerns in this sub a bit more into separate subs and functions, following the single responsibility principle, although, admittedly, the sub already does not do all too many things.


Updated: Questions on implementing Builder design pattern I am in the process of using Java Builder design pattern to write a connector class that has optional member variables and need some suggestions. My question from here Standard practices for employing Java Builder design pattern has been updated and the code now presented is not psuedocode but is specific.

public class Connector {

    // Instance variables
    String appUrl = null;
    String oneTimePwd = null;
    String accessKey = null;

    // Private constructor
    private Connector(Builder builder) {
        this.appUrl = builder.appUrl;
        this.accessKey = builder.accessKey;
        this.oneTimePwd = builder.oneTimePwd;
    }

    public static class Builder {

        // Instance variables
        String appUrl = null;
        String accessKey = null;
        String oneTimePwd = null;

        public Builder() {
            // Empty constructor that accepts no arguments.
        }

        // Setter method for mandatory field/s
        public Builder setMandatoryParameters(String appUrl) {
            this.appUrl = appUrl;
            return this;
        }

        // Setter method1 for optional field/s
        public Builder setAccessKey(String accessKey) {
            this.accessKey = accessKey;
            return this;
        }

        // Setter method2 for optional field/s
        public Builder setOneTimePwd(String oneTimePwd) {
            this.oneTimePwd = oneTimePwd;
            return this;
        }

        public Connector Build() {
            if (accessKey != null) {
                return new Connector(this);
            } else if (oneTimePwd != null) {
                return new Connector(this);
            } else {
                // log.error("Either Access key or One time password must be specified.")
            }
            return null;
        }
    }

    // Other methods of the Connector class
    // ...
}

Question 1:
Is it sensible to make use of a method "setMandatoryParameters()" to obtain values for mandatory parameters? This will allow creation of an instance of the Builder without supplying the mandatory parameters which can be a drawback. Is it much better to ask the user to supply the values for mandatory parameters with Builder's constructor?

Question 2:
Either access key or one time password is needed to establish a successful connection. Thus, field validations such as null checks have been employed in the "Build()" method. Should such validations be performed in the Connector class's constructor instead?

Question 1: If the parameters are mandatory anyway you might as well enforce this through your API. This reads just fine:

Connector con = new Connector.Builder(mandatoryURL)
                    .setAccessKey(key)
                    .build();

Question 2: Either place, since you contained the checks entirely in the Connector class (because of the private constructor) you have full control anyway.

Since either one of them is mandatory and you I'm assuming you won't use both of them, you might also provide 2 separate "build" methods that take one of those parameters as input.

public static class Builder {
    String appUrl = null;

    public Builder(String appUrl) {
        this.appUrl = appUrl;
    }

    public Connector withAccessKey(String accessKey) {
        Connector result = new Connector(appUrl);
        connector.accessKey = accessKey;
        return connector;
    }

    public Connector withOneTimePwd(String oneTimePwd) {
        Connector result = new Connector(appUrl);
        connector.oneTimePwd = oneTimePwd;
        return connector;
    }
}

Quick note that I changed the Connector constructor to also take the URL parameter instead of the builder. Your way works just fine in this case, I just prefer to have the builder be responsible for setting the correct fields instead of making the Connector "know" about the Builder class.

Alternative solution to using a Builder is to use factory methods. Your Connector class could then look something like this:

public class Connector {

    String appUrl = null;
    String oneTimePwd = null;
    String accessKey = null;

    private Connector(String appUrl) {
        this.appUrl = appUrl;
    }

    public static Connector createWithAccessKey(String appUrl, String accessKey) {
        Connector result = new Connector(appUrl);
        connector.accessKey = accessKey;
        return connector;
    }

    public static Connector createWithOneTimePwd(String appUrl, String oneTimePwd) {
        Connector result = new Connector(appUrl);
        connector.oneTimePwd = oneTimePwd;
        return connector;
    }

    //... rest of the class
}

You can now use it as follows:

Connector con = Connector.createWithAccessKey(someUrl, myKey);

This has about the same effect as your Builder idea but with a lot less boilerplate code to reach it.


A simple Decryption Program using Python This is a program that can decrypt the codes that have been encrypted using the algorithm that my previously posted encryption program follows, you just need to enter the encrypted text from that program.

I got a lot of suggestions from people on the encryption post, I have tried to make this program as short as possible (in my efforts), I did not use the method they suggested to shorten functions because I don't want to use it without understanding how it works, I will implement it in my further programs as soon as I understand it...

Thank You for the suggestions on the previous program, it helped me reduce the length of this program by 16 lines.

Please let me know what you think about this program

Python Code :

alphabets = 'abcdefghijklmnopqrstuvwxyz'

def position_in_alphabets(tofind):
    for i in range(0,26):
        if tofind == alphabets[i]:
            position = i+1
            break
    return position

def decrypt(a):
    output = ''
    for i in range(0,len(a)):
        character = a[i]
        z = i+1
        y = position_in_alphabets(character)
        x = z+y-26
        if x>len(alphabets):
            x = x % len(alphabets)
        alpha = alphabets[x-1]
        output += alpha
    return output

print ()
print ("NOTE : Please enter just lowercase characters (no special characters) and no spaces")
print ()
given = input("Please enter the word to be decrypted : ")
output = decrypt(given)
print ()
print ("The word which is coded as ",given," is : ",output)

Thank You :)

Use built in methods!

Your position_in_alphabets function can be reduced to one line. In fact, replace y = ... with the following:

alphabets.index(character) + 1

index returns the first occurrence of the character in question. Since you're working with the alphabet, it will return the position of the character in that string. Then you just need to add one.

Never trust user input

Instead of trusting the user will only enter lowercase characters after telling them to do so, call .lower() on the input to change the string to all lowercase letters.

I'm going to step through each line of the main function now.

f"" strings

I would use f"" strings so you can directly format your variables in your strings. In this case, it reduces the clutter that print("...", var, "...", var, "...") causes. It would instead be print(f"... {var} ... {var} ...").

Naming conventions (PEP 8)

a isn't really a good parameter name. A variable/parameter name should represent what that variable is holding or is doing. I would use string, since you're working with them in this program.

def decrypt(string):

Use enumerate

enumerate allows you to work through an iterable with the index and the value at that position in the iterable. It gets rid of having to iterable[i] and all that junk.

output = ''
spaces = 0
for index, character in enumerate(string):

Allowing spaces

Spaces are really easy to deal with. If the character is a space, simply add " " to the output and continue, quite literally, to the next iteration.

if character == " ":
    output += " "
    spaces += 1
    continue

Compress your computations

Instead of having a bunch of one character variable names x, y, z, I would do all the computations in one step and assign them to a variable. I've done so, and used position, because that's what you're calculating.

position = (index + 1) + (alphabet.index(character) + 1) - 26

Simplify your reassignments

Have a look at this:

if position > len(alphabet):
    position %= len(alphabet)

It's the same thing as position = position % len(alphabet), but shorter (also called in place modulus). It also works for the other operators (+-*/).

Don't make variables you don't need

The sole purpose of alpha is to hold the value at the passed position in alphabet, then you add alpha to output. How about you just skip the first step and add it right to output?

output += alphabet[position - 1]

All in all, your final program looks like this:

alphabet = 'abcdefghijklmnopqrstuvwxyz'

def decrypt(string: str) -> str:
    output = ''
    spaces = 0
    for index, character in enumerate(string):
        if character == " ":
            output += " "
            spaces += 1
            continue
        position = (index + 1) + (alphabet.index(character) + 1) - len(alphabet)
        if position > len(alphabet):
            position %= len(alphabet)
        output += alphabet[position - 1 - spaces]
    return output

word = input("Please enter the word to be decrypted: ").lower()
output = decrypt(word)
print(f"The word '{word}' decrypted is '{output}'")

We use spaces to shift the index back for every space we've encountered.


JavaScript MVC - review and suggestions Before I start working on a quite JavaScript heavy application I would like you to review my JS MVC approach.

How can it be improved?
Are there any mistakes/issues I should be aware of?

Since the entire app will have multiple modules I would like to split the code into standalone modules that can be loaded as required (any suggestions how to do this?)

html

<!DOCTYPE html>

            <input type="button" id="button" value="world"/>

project.js

var GLOBAL = {} || GLOBAL;

GLOBAL.SETTINGS = function($) {

    var global = {
        fontColor:"red"
    }

    return {
        MODEL : {
            text : "Text Inside The Containter"
        },

        VIEW : {
            container: $(".container")
        },

        CONTROLLER : {
            style : function() {
                GLOBAL.SETTINGS.VIEW.container.css("color",global.fontColor).append("
" + GLOBAL.SETTINGS.MODEL.text + "
");
            }
        },

        init : function() {
            GLOBAL.SETTINGS.CONTROLLER.style();
        }
    }
}(jQuery);

GLOBAL.SETTINGS.init();

utils.js

GLOBAL.UTILS = function($) {

var text = "SOMETHING"; //local

    return {
        MODEL : {
            text : "World"
        },

        VIEW : {
            button :   $("#button"),
            container: $("#message")
        },

        CONTROLLER : {
            hello : function(msg) {
                GLOBAL.UTILS.VIEW.container.append( "Hello " + GLOBAL.UTILS.MODEL.text + "
" );
            }
        },

        EVENTS : function() {
            GLOBAL.UTILS.VIEW.button.click( function() {
                GLOBAL.UTILS.CONTROLLER.hello();
            });
        },

        init : function() {
            GLOBAL.UTILS.EVENTS();
        }
    }
}(jQuery);

GLOBAL.UTILS.init();

Since the entire app will have multiple modules I would like to split
  the code into standalone modules that can be loaded as required (any
  suggestions how to do this?)

Use a dynamic module loader such as requireJS. You can use it to load modules dynamically using AJAX, and it features an optimization tool to combine your scripts into a single file to reduce the number of HTTP requests on production server.

  How can it be improved?

Stick to shared conventions for the naming of variables. Do not use all-uppercase for regular variables, only when you wish to indicate a constant value.

Use a name specific to your appliation/website, organization or company instead of GLOBAL for the global variable referencing the root of your library, if any: it is no longer needed using Asynchronous Module Definition with requireJS.

  Are there any mistakes/issues I should be aware of?

You should think about the way you wish to handle the communication between modules. The latest trend is to abstract and decouple the communication using publish/subscribe pattern.

You can find more details in The Scalable JavaScript Application Architecture presentation by Nicholas C. Zakas, and you may be interested in the open-source framework that I developed based on this approach, which I presented recently in Paris JavaScript user group.


Sum and average calculator I've just written some code in an online compiler, and since I'm very new to C#, I wanted to ask what could be improved about the code.

The task of the program is simply to add up the numbers the user entered before and then calculate the average.

public static void Main()

{
    int input;
    int sum = 0;
    bool error = false;
    int[] numbers = new int[5];

    for(int i = 0; i < numbers.Length; i++)
    {

        if (!error)
            Console.WriteLine("Please write a number:");

        if(Int32.TryParse(Console.ReadLine(), out input))
        {
            numbers[i] = input;
            sum += numbers[i];
            error = false;
        }
        else
        {
            Console.WriteLine("An error occured, please try again:");
            error = true;
            i--;
            continue;
        }
    }

    Console.WriteLine("");
    Console.WriteLine("Your typed in values:");
    for(int i = 0; i < numbers.Length; i++)
    {
        Console.WriteLine(numbers[i]);
    }

    Console.WriteLine("");

    Console.WriteLine("The result of your numbers:");
    Console.WriteLine(sum);

    Console.WriteLine("The average of your numbers:");
    Console.WriteLine((double)sum / (double)numbers.Length);
}

The code works fine, but maybe I could do some things more effectively.
Would be glad to get some help :)

Just to sum things up here is the console-output:

Please write a number:
> 4
Please write a number:
> 2
Please write a number:
> a
An error occured, please try again:
> 6
Please write a number:
> 4
Please write a number:
> 8

Your typed in values:
4
2
6
4
8

The sum of your numbers:
24
The average of your numbers:
4.8

('>' = User input)

The code works, and it seems that you're take care of invalid input. But the logic of the workflow is not - well - that logic.

  int[] numbers = new int[5];

Here 5 is a magic number. Why five? Why not four or six or 20?

You should announce the number of input before asking for it (and maybe why).

Alternatively you could let the user enter a number of numbers to be calculated.

int sum = 0;

You probably have the sum variable for optimization reasons. Alternatively you could let the built in extension Sum() handle that:

numbers.Sum();

  if (!error)

You have this negation of a "negative" variable. Why not be positive and call it success:

if (success)
  Console.Write("Enter Number: ");

Instead of:

    if (Int32.TryParse(Console.ReadLine(), out input))
    {
      numbers[i] = input;
      sum += numbers[i];

you could write:

    success = Int32.TryParse(Console.ReadLine(), out input);

    if (success)
    {
      ...
    }
    else
    {
      ...
    }

      continue;

I don't see any reason for calling continue in your loop? You use continue to step over the rest of the loop and reenter it - if the condition is still true, but here it is the last expression in the loop, so there is nothing to skip.

According to the overall design you should always split your code into meaningful methods in order to accommodate to principles like DRY and Single Responsibility:

For instance:

void Main()
{
  if (!GetCountOfNumbers(out int numCount))
    return;

  if (!GetUserInput(numCount, out int[] numbers, out int sum))
    return;

  PrintValues(numbers);
  Console.WriteLine("");
  PrintResult(sum, numbers.Length);
  Console.WriteLine("");
}

Where:

private bool GetCountOfNumbers(out int numCount)
{
  if (!GetIntInput("Enter count of Numbers", "Must be a positive number", x => x > 0, out numCount))
  {
    Console.WriteLine("User aborted the calculation");
    return false;
  }

  return true;
}

and

private bool GetUserInput(int numCount, out int[] numbers, out int sum)
{
  numbers = new int[numCount];
  sum = 0;
  int input;

  for (int i = 0; i < numCount; i++)
  {
    if (!GetIntInput($"Enter number {i + 1}", "Enter any valid integer (32 bit) value", x => true, out input))
    {
      Console.WriteLine("User aborted the calculation");
      return false;
    }

    numbers[i] = input;
    sum += input;
  }

  return true;
}

Both methods are calling GetIntInput() which could be defined as follows:

private bool GetIntInput(string message, string errorMessage, Predicate<int> predicate, out int result)
{
  result = default;

  while (true)
  {
    Console.Write($"{message} ['q' to Exit]: ");
    string input = Console.ReadLine();
    if (input == "q" || input == "Q")
      return false;

    if (int.TryParse(input, out int number) && predicate(number))
    {
      result = number;
      return true;
    }
    Console.WriteLine($"Invalid input: {errorMessage}");
  }
}

It takes a message to the user, a predicate to validate the input against and an error message if the predicate returns false on the input, and it returns the result in an out variable (result). Further it allows the user to exit the input loop and then returns true or false accordingly.

In this design everything is counted for through descriptive method names and the logic in the workflow is easily understood and separately maintainable.

It could surely be done in more sophisticated ways, but as a simple user input driven console application like this, the above is a simple way to structure the code properly.


Extract String From Text (Word) I need to find a specific string of text from a large source of material. I've just used Excel VBA up to this point, so I don't know the Word objects and I ended up just recording some macros and working with what I had. However, this way is very inefficient. I looked on Stack Overflow, but I didn't understand what they were doing, so I just did it this way.

What I am mostly looking for is the objects used in Word VBA that are equivalent to the Excel 'Range', 'Cells', 'Row', and 'Column'.  I looked on MSDN, but there is so much material there that you need to have an idea of what you are looking for, and I couldn't find any other sites that would help.

This is what I wrote, and it did the job, but it is very inefficient.

Sub FindMediaInBraces()

    ' Separate needed text
    Selection.HomeKey Unit:=wdStory
    With Selection.Find
        .Text = "[Media"
        .Replacement.Text = "^p^p[Media"
        .Forward = True
        .Wrap = wdFindContinue
    End With
    Selection.Find.Execute Replace:=wdReplaceAll

    Selection.HomeKey Unit:=wdStory
    With Selection.Find
        .Text = ".jpg]"
        .Replacement.Text = ".jpg]^p^p"
        .Forward = True
        .Wrap = wdFindContinue
    End With
    Selection.Find.Execute Replace:=wdReplaceAll

    Selection.HomeKey Unit:=wdStory
    With Selection.Find
        .Text = ".png]"
        .Replacement.Text = ".png]^p^p"
        .Forward = True
        .Wrap = wdFindContinue
    End With
    Selection.Find.Execute Replace:=wdReplaceAll

    ' Remove text that doesn't start correctly
    Dim i As Long

    Selection.HomeKey Unit:=wdStory
    For i = 1 To ActiveDocument.Paragraphs.count
        Selection.MoveDown Unit:=wdParagraph, count:=1, Extend:=wdExtend
        With Selection.Find
            .Execute FindText:="[Media"
            .Forward = True
            If .Found = True Then
                Selection.MoveDown Unit:=wdParagraph, count:=1
            Else
                Selection.Delete Unit:=wdCharacter, count:=1
            End If
        End With
    Next

End Sub

Example Text

Original

Type: MC
Objectives: 2.6
16) [HTML]If y varies directly with x, write an equation for the direct variation. Then find the value. If y = -4 when x = -28, what is the value of x when y = 5?[/HTML]
*a. `$y=\frac{1}{7}x;\,\, 35$`
b. `$y=-\frac{1}{7}x;\,\, -35$`
c. `$y=7x;\,\, \frac{5}{7}$`
d. `$y=-7x;\,\, -\frac{5}{7}$`

Type: MC
Objectives: 3.3
21) Select the graph that correctly represents the following system of equations:
  `$y=-\frac{5}{3}x+3$`
  `$y=\frac{1}{3}x-3$`
*a. [Media:Media/Images/Problem 21a.jpg]
b. [Media:Media/Images/Problem 21b.jpg]
c. [Media:Media/Images/Problem 21c.jpg]
d. [Media:Media/Images/Problem 21d.jpg]

After Macro

[Media:Media/Images/Problem 21a.jpg]
[Media:Media/Images/Problem 21b.jpg]
[Media:Media/Images/Problem 21c.jpg]
[Media:Media/Images/Problem 21d.jpg]

This is what I found on Stack Overflow, but I don't know the objects to use to apply it, and I don't know if it would actually work in this case.

dim str as string
dim openPos as integer
dim closePos as integer
dim midBit as string

str = "NUMBER(8,3)"
openPos = instr (str, "(")
closePos = instr (str, ")")
midBit = mid (str, openPos+1, closePos - openPos - 1)

A good way to do this would be by .Sentences, assuming your string doesn't appear more than once per sentence. You can use instr to get the location of your strings and then store them in an array. Then print them wherever you want:

Sub GetMediaStrings()
    Const BEGIN_STRING As String = "[Media:"
    Const END_STRING As String = ".jpg"
    Dim searchRange As Range
    Dim startString As Long
    Dim endString As Long
    Dim results() As String
    ReDim results(1 To 2)
    For Each searchRange In ActiveDocument.Sentences
        startString = InStr(1, searchRange, BEGIN_STRING, vbTextCompare)
        If startString > 0 Then
            endString = InStr(1, searchRange, END_STRING, vbTextCompare)
            If endString > 0 Then
                results(UBound(results) - 1) = Mid(searchRange, startString, endString - startString + Len(END_STRING))
                ReDim Preserve results(1 To UBound(results) + 1)
            End If
        End If
    Next
End Sub

You might also want to do it via .StoryRanges.


Array partition 1 to group the array into pairs and make sum of minimum of pairs as maximum Challenge from LeetCode: Array Partition I

  Given an array of 2n integers, your task is to group these integers into n pairs of integer, say (a1, b1), (a2, b2), ..., (an, bn) which makes sum of min(ai, bi) for all i from 1 to n as large as possible.

  Example 1:

  Input: [1,4,3,2]

  Output: 4

  Explanation: n is 2, and the maximum sum of pairs is 4 = min(1, 2) + min(3, 4).

My approach:

import java.util.*;

class Solution {
    public int arrayPairSum(int[] nums) {
       //Sorting the array in least time
        Arrays.sort(nums);

        int sum = 0;
        for( int i = 0; i < nums.length; i = i + 2 )
            sum += nums[i];

        return sum;

    }
}

1) Is there a better way to solve this question?

2) Can I still decrease the time and space complexity?

Reference

Your logic is correct. You need each pair to include numbers as close as possible together, so that when you sacrifice each big number in the pair, the little number that survives is as large as possible while still being small enough to be shielded by the big number. The obvious way to pair up numbers like that is to sort them, and for the problem you've describe the language library sorting algorithm is probably the best choice.

There are still a few things that want cleaning up with this solution.

First, you are mutating your input array by sorting it. That is probably an undesirable side effect for this function, and should be avoided.

Second, it is often worth checking promised preconditions that you're relying on are actually honoured by whoever is using the function. This is where exceptions are useful. For example, I would check that the length of the input array is even.

Third, it is generally undesirable to use the import something.* form because it leads to confusion about where things are coming from.

Following the link, there is a further guarantee that

  n is a positive integer, which is in the range of [1, 10000]. All the
  integers in the array will be in the range of [-10000, 10000].

An array of size 20000 is plausible to hold in memory. This means that Counting sort is a plausible alternative to the sort from the Language library, and it has time complexity linear in the range of possible values, whereas the quicksort implementation used by Java is loglinear in the number of elements. (You don't have to unpack the count bins back into a sorted array, but can directly work out the pairs and smaller of the pairs)

In practice for the input constraints given I would still expect the quicksort to be faster and all round cheaper. However, I don't have any profiling on that; it's just a guess. Further, if the range of possible integers was very much smaller or the value of n was very much larger, implementing a counting approach would probably work well.

(Strictly speaking because, as above, you'd need to copy the input array to avoid mutating your input with a sort, given n is larger than the range of the values, an array of counts would be cheaper in terms of space too!)


Using vswprintf( ... ) to create wchar_t* with variable argument list I am using c++17 and wide characters.
I have created a function to create a wchar_t* using a variable number of parameters ...
#include <stdarg.h>

// the caller will free the memory
wchar_t* GetMessage( const wchar_t* format, ... )
{
  va_list args;
  va_start(args, format);

  // get the size of the final string
  const auto size = vswprintf(nullptr, 0, format, args);

  // create the buffer
  const auto buffSize = size + 1;
  const auto buffer = new wchar_t[buffSize];
  memset(buffer, 0, buffSize * sizeof(wchar_t) );

  // create the string
  vswprintf_s(buffer, buffSize, format, args);

  va_end(args);

  // all done
  return buffer;
}

Can you suggest a more efficient, standard, way of achieving the above?

The code you have written is fine, apart from three small issues:

You need to check that format isn't null before calling vswprintf otherwise it crashes.
There seems to be the potential for memory leaking, you are assuming the caller will always release the memory.  It would return a std::wstring instead, no need to new the buffer and it is "automatically" released.
Check the results of vswprintf_s() it will almost never fail, but when it does you'll be trying to track down what went wrong for days. :)

This is my version:
// the caller will free the memory
std::wstring getMessage(const wchar_t* format, ...)
{
    std::wstring output;                                            // function return value.
    if (format != nullptr)
    {
        va_list args;
        va_start(args, format);
        const auto size = vswprintf(nullptr, 0, format, args);      // get the size of the final string
        if (size > 0)                                               // If the above call worked
        {
            const auto buffSize = 1 + size;
            output.reserve(buffSize);                               // create the buffer
            if (vswprintf_s(output.data, buffSize, format, args) < 0)// create the string
            {
                output.clear();                                     // Empty the string if there is a problem
            }
        }
        va_end(args);
    }
    return output;
}


Code review needed for my Prime factorization code Below is the code for knowing all the prime factors of a number. I think there must be some better way of doing the same thing. Also, please tell me if there are some flaws in my code.

def factorize(num):
    res2 = "%s: "%num
    res = []
    while num != 1:
        for each in range(2, num + 1):
            quo, rem = divmod(num, each)
            if not rem:
                res.append(each)
                break
        num = quo
    pfs = set(res)
    for each in pfs:
        power = res.count(each)
        if power == 1:
            res2 += str(each) + " "
        else:
            res2 += str(each) + '^' + str(power) + " "
    return res2

def factorize(num):
    res2 = "%s: "%num

You'd be much better off to put the string stuff in a seperate function. Just have this function return the list of factors, i.e. res and have a second function take res as a parameter and return the string. That'd make your logic simpler for each function.

    res = []

Don't needlessly abbreviate. It doesn't save you any serious amount of typing time and makes it harder to follow your code.

    while num != 1:
        for each in range(2, num + 1):

To be faster here, you'll want recompute the prime factors of the numbers. You can use the Sieve of Eratosthenes. Just keep track of the prime factors found, and you'll just be able to look them up.

            quo, rem = divmod(num, each)
            if not rem:
                res.append(each)
                break
        num = quo

Your function dies here if you pass it a zero.

    pfs = set(res)

Rather than a set, use collections.Counter, it'll let you get the counts at the same time rather then recounting res.

    for each in pfs:
        power = res.count(each)
        if power == 1:
            res2 += str(each) + " "
        else:
            res2 += str(each) + '^' + str(power) + " "

String addition is inefficient. I'd suggest using "{}^{}".format(each, power). Also, store each piece in a list and then use " ".join to make them into a big string at the end.

    return res2


C++ Optional Reference Here is a good article on an optional reference type in C++. They discuss std::optional<T&>, but as that doesn't compile I have made my own.

One purpose of this type is to remove raw pointers from function signatures (where references alone cannot be used), as a raw pointer does not convey any indication of what it will be used for (become owned, deleted, iterated, dereferenced, etc).

#include <functional>
#include <optional>

template<typename T>
/** @brief An optional refference **/
class opt_ref {
    using std_opt_ref = std::optional<std::reference_wrapper<T>>;
    std_opt_ref data = std::nullopt;
public:
    using type = typename std::reference_wrapper<T>::type;

    /** public member functions **/

    T& get() { return data.value().get(); }
    const T& get() const { return data.value().get(); }
    bool has_value() const { return data.has_value(); }
    T& value_or(T&& other) const { return data.value_or(other); }

    /** constructors **/

    opt_ref() {}
    opt_ref(T& source) : data(source) {}
    opt_ref& operator = (T&& other) { data.value().get() = other; return *this; }

    /** comparisons **/

    bool operator == (const T& t) { return data.value() == t; }
    bool operator == (const std::nullopt_t&) {return !data.has_value(); }

    /** implicit conversion **/

    operator T&() { return data.value().get(); }
    operator const T&() const { return data.value().get(); }
    operator std::reference_wrapper<T>() { return data.value(); }
    operator const std::reference_wrapper<T>() const { return data.value(); }
};

Design

References are different from pointers in two ways:

they are designed to be aliases to the objects they refer to, so syntactically they are treated with special care;
they cannot be rebound.

You cannot always emulate the first bullet with an optional — for example, there's no general way to make opt.f() call opt.value().f().  You still have to resort to some other syntax like opt->value().  Therefore, my advice is to simply treat opt_ref<T> like an immutable nullable pointer that does not own the referred-to object — don't follow std::reference_wrapper.

Code Review

using type = typename std::reference_wrapper<T>::type;

typename std::reference_wrapper<T>::type is just T.  Also, the standard terminology is value_type.

T& get() { return data.value().get(); }
const T& get() const { return data.value().get(); }
bool has_value() const { return data.has_value(); }
T& value_or(T&& other) const { return data.value_or(other); }

has_value is noexcept.  Why does value_or take an rvalue reference?  To introduce dangling references as in opt.value_or(1)?  Take an lvalue reference instead.

bool operator == (const T& t) { return data.value() == t; }
bool operator == (const std::nullopt_t&) {return !data.has_value(); }

I'm not sure this is the right approach.  The first == compares values (and throws an exception if there is no value), whereas the second == compares the references themselves.  You can imitate the behavior of std::optional:

bool operator==(const opt_ref<T>& a, const opt_ref<T>& b)
{
    if (a.has_value() != b.has_value()) {
        return false;
    } else if (!a.has_value()) { // and !b.has_value()
        return true;
    } else {
        return a.get() == b.get();
    }
}

operator T&() { return data.value().get(); }
operator const T&() const { return data.value().get(); }
operator std::reference_wrapper<T>() { return data.value(); }
operator const std::reference_wrapper<T>() const { return data.value(); }

As I said before: are you sure you want this (especially the implicit conversions to reference_wrapper)?

Other functionalities

Consider:

operator* and operator->;
explicit operator bool;
has_value;
...


Algorithm to add all lucky numbers under [N] to a Vector A lucky number is defined as a positive integer whose decimal representation contains only the lucky digits 4 and 7. For example, numbers 47, 744, 4 are lucky and 5, 17, 467 are not. I need to check if a given number is evenly divisible by any lucky number or not.
Now, suppose I want to add all Lucky Numbers under a given integer [N] to a vector, without using recursion. For the sake of simplicity, let N = 1000.

I came up with an approach to just check each digit of all the numbers under [N] by making separate loops for 1 digit numbers, 2 digit numbers etc.
for(int number=0;number<10;number++) {if(((number%10==4)||(number%10==7))) {lucky.push_back(number);}} //1 Digit Lucky Numbers
for(int number=10;number<100;number++) {if(((number%10==4)||(number%10==7))&&(((number/10)%10==7)||((number/10)%10==4))) {lucky.push_back(number);}} //2 Digit Lucky Numbers
for(int number=100;number<1000;number++) {if(((number%10==4)||(number%10==7))&&(((number/10)%10==7)||((number/10)%10==4))&&(((number/100)%10==7)||((number/100)%10==4))) {lucky.push_back(number);}} //3 Digit Lucky Numbers
for(int number=10;number<100;number++) {if(((number%10==4)||(number%10==7))&&(((number/10)%10==7)||((number/10)%10==4))&&(((number/100)%10==7)||((number/100)%10==4))&&(((number/1000)%10==7)||((number/1000)%10==4))) {lucky.push_back(number);}} //4 Digit Lucky Numbers

I was thinking that this could roughly be converted to something along these lines but I am not quite able to come up with what exactly to do.
for(number;number<10*itr_counter;number++)
    {
    if(((number%10*itr_counter==4)||(number%10*itr_counter==7))) {lucky.push_back(number);}
    itr_counter*=10;
    }

I basically want to check each digit of all 1 digit numbers by taking modulo 10 and checking if the digits are 4 or 7. Similarly for a number consisting of X digits, I am taking modulo and dividing the number by 10, 100 and so on to check against 4 or 7.
Can someone help me optimise the first block of code into something smaller and more efficient? Something along the lines of the second block of code would work.
The Program
#include <bits/stdc++.h>
using namespace std;

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

//This Part Needs to be Optimised
vector<int>lucky;
for(int number=0;number<10;number++) {if(((number%10==4)||(number%10==7))) {lucky.push_back(number);}} //1 Digit Lucky Numbers
for(int number=10;number<100;number++) {if(((number%10==4)||(number%10==7))&&(((number/10)%10==7)||((number/10)%10==4))) {lucky.push_back(number);}} //2 Digit Lucky Numbers
for(int number=100;number<1000;number++) {if(((number%10==4)||(number%10==7))&&(((number/10)%10==7)||((number/10)%10==4))&&(((number/100)%10==7)||((number/100)%10==4))) {lucky.push_back(number);}} //3 Digit Lucky Numbers
for(int number=10;number<100;number++) {if(((number%10==4)||(number%10==7))&&(((number/10)%10==7)||((number/10)%10==4))&&(((number/100)%10==7)||((number/100)%10==4))&&(((number/1000)%10==7)||((number/1000)%10==4))) {lucky.push_back(number);}} //4 Digit Lucky Numbers
//This Part Needs to be Optimised

int flag=0;
for(unsigned int index=0;index<lucky.size();index++)
    {
    if(in_num%lucky[index]==0) {flag=1; break;}
    }
if(flag==1) {cout<<"YES";}
else {cout<<"NO";}
return 0;
}

It is fairly straightforward to generate a vector with all the values.  First, you should treat all the same length numbers as a group.  The values 4 and 7 would be in a singledigit vector.  To get the twodigit vector, simply go through the previous vector, multiplying each by 10 and then adding 4 to push back a value and add 7 to push back another value.

vector<int> lucky;
vector<int> current;
vector<int> next;
current.push_back(4);
current.push_back(7);
lucky.push_back(4);
lucky.push_back(7);
int value=0;
while(value < N)
{
   for(unsigned int i=0;i<current.size();++i)
   {
       value=current[i]*10+4;
       next.push_back(value);
       lucky.push_back(value);
       value=current[i]*10+7;
       next.push_back(value);
       lucky.push_back(value);
   }

   current=next;
   next.clear();
}


Close modal and open new modal I am trying to create a script that shows my login modal on page load. I have a button that opens the register modal and closes the login modal if clicked.

Here is the script I am using:

$(document).ready(function(){
  $('#loginmodal').modal('show')
});

$(".hidelogopenreg").click( function()
         {
            $('#loginmodal').modal('hide')
            $('#registermodal').modal('show')
         }
 );

Even though the works correctly, I fear it's using bad practices as I have little JavaScript knowledge and don't want to have to resolve problems further down the line.

Is this correct and efficient or is there a better way of writing this?

Another problem I had is that hiding a modal causes the body to shift to the right on close, to fix this I added:

body {
padding-right:0px !important;
margin-right:0px !important;
}

Could this also cause problems for me?

Could this also cause problems for me?

As with most CSS questions, the answer is maybe. It really depends on the rest of your CSS. Personally, I try to stay away from !important but I can certainly understand your usage. As long as it isn't causing additional issues, I don't see a problem with it.

  Is this correct and efficient or is there a better way of writing this?

I would recommend wrapping your code in an IIFE so as not to pollute the global scope and give yourself a "safe" private scope.  This also allows you to use $ for jQuery with no conflicts:

(function( $ ) {
  //code here
})( jQuery );

Next, if you see where you use the same jQuery selection more than once, you should just go ahead and cache it:

var logModal = $('#loginmodal');

I would also recommend that you use K&R style braces instead of Allman style.  This can help prevent issues you might encounter with JS.

$(".hidelogopenreg").click( function() {  // opening on same line K&R style
  $('#loginmodal').modal('hide')
  $('#registermodal').modal('show')
}

Lastly, you need to end your lines of code with a semicolon. You can get away with this in some situations but can cause issues later on. More information here and here.

  $('#loginmodal').modal('hide');  //end of line
  $('#registermodal').modal('show');  //end of line

Here is the code with these changes:

(function( $ ) {

  var $logModal = $('#loginmodal');
  var $regModal = $('#registermodal');
  var $hideLogin = $(".hidelogopenreg");

  function swapModals() {
    $logModal.modal('hide');
    $regModal.modal('show');
  }

  $(document).ready( function() {
    $logModal.modal( 'show');
    $hideLogin.on( 'click', swapModals );
  });

})( jQuery );

Hope that helps.  Let me know if you have questions, comments or need clarification.


OleDbContext to keep OleDbConnection alive I'm writing some automated tests that read and write data from/to excel (so end-users can define their test cases themselves). I use dotTratce to optimize the algorithms and found that the worst bottleneck was writing to excel. It turns out that OleDb does not have connection pooling and opening/disposing it takes a lot of time. Even with only 200 writes it sums up to 80 seconds (opening 40sec and dispose 40sec). To fix this I wrote this simple OleDbContext that I pass to methods that write to excel so that they don't open/close a connection for each write.

public class OleDbContext : IDisposable
{
    private OleDbConnection _connection;

    public OleDbContext(string fileName)
    {
        FileName = fileName ?? throw new ArgumentNullException(nameof(fileName));
    }

    public string FileName { get; }

    public void Connect()
    {
        if (_connection == null)
        {
            _connection = new OleDbConnection(CreateConnectionString(FileName));
            _connection.Open();
        }
    }

    public int Execute(Func<OleDbConnection, int> query)
    {
        if (_connection == null) { throw new InvalidOperationException("There is no open connection yet. You need to connect first."); }
        return query(_connection);
    }

    private static string CreateConnectionString(string fileName)
    {
        return $"Provider=Microsoft.ACE.OLEDB.12.0;Data Source={fileName};Extended Properties='Excel 12.0 Xml;HDR=No;'";
    }

    public void Dispose()
    {
        if (_connection != null)
        {
            _connection.Dispose();
            _connection = null;
        }
    }
}

Usage example:

public int Write(string text, string columnLetter, int rowNumber, OleDbContext oleDbContext)
{
    return oleDbContext.Execute(conn =>
    {
        var range = $"{columnLetter}{rowNumber}";
        using (var update = new OleDbCommand($"UPDATE [{SheetName}${range}:{range}] SET [F1]='{text}'", conn))
        {
            return update.ExecuteNonQuery();
        }
    });
}

With this improvement the execution time dropped to under 2 seconds which is very good. I was just wondering if there's more I can improve - performance wise I guess I did what is technically possible so how about other things?

It turns out that OleDb does not have connection pooling and
  opening/disposing it takes a lot of time.

I have read somewhere, that as long as you keep one connection (for sure with the exact same connection string) open the connection pooling works, but don't count on it, just test it. You could at test initialize open this said connection and close it at test terminate.

Now let us look at your code.... well, there isn't much to say. It is easy to read, you are using braces {}, fields, methods and method parameters are well named.

Just some little nitpicks:

what happens if the constructor argument is an empty string ?
you are testing excel files, why don't you ensure that the passed fileName has at least the correct file extension ?


DRYing out some Java code and tests I am writing some tests that all follow the same pattern, I feel like repeating myself over and over again, how can I reduce the repetition? The (almost) repeating part is in constructor and the class level variables:

VariablesPage

public class VariablesPage extends LoadableComponent<VariablesPage> {

    private final TestHelpers helpers;
    private WebDriver driver;

    @FindBy(xpath = "//a[@title=\"Define new variable\"]")
    private WebElement btnAddNewVariable;

    public VariablesPage() throws Exception {
        helpers = new TestHelpers();
        driver = helpers.getWebDriver();
        helpers.setHar(VariablesPage.class.getSimpleName());
        PageFactory.initElements(driver, this);
        isLoaded();
        helpers.getHar();
    }

    @Override
    protected void load() {

    }

    @Override
    protected void isLoaded() throws Error {
        helpers.waitUntil(btnAddNewVariable);
    }
}

addStepsPage

public class AddStepsPage extends LoadableComponent<AddStepsPage> {

    private final TestHelpers helpers;
    private WebDriver driver;
    @FindBy(xpath = "//div[@class=\"body sequential\"]")
    private WebElement divStepsBody;

    public AddStepsPage() throws Exception {
        helpers = new TestHelpers();
        driver = helpers.getWebDriver();
        helpers.setHar(AddStepsPage.class.getSimpleName());
        PageFactory.initElements(driver, this);
        isLoaded();
        helpers.getHar();
    }

    @Override
    protected void load() {

    }

    @Override
    protected void isLoaded() throws Error {
        helpers.waitUntil(divStepsBody);
    }
}

First, let us collect the code pieces which are repeated

Fields

private final TestHelpers helpers;
private WebDriver driver;

Constructor related

helpers = new TestHelpers();
driver = helpers.getWebDriver();

Now, let's start refactoring by extending LoadableComponent<T>

public abstract class ExtendedLoadableComponent<T> extends LoadableComponent<T> {

    protected final TestHelpers helpers
    protected WebDriver driver;

    public ExtendedLoadableComponent() throws Exception {
        helpers = new TestHelpers();
        driver = helpers.getWebDriver();
    }
}

which reduces the first class to

public class VariablesPage extends ExtendedLoadableComponent<VariablesPage> {

    @FindBy(xpath = "//a[@title=\"Define new variable\"]")
    private WebElement btnAddNewVariable;

    public VariablesPage() throws Exception {
        super();
        helpers.setHar(VariablesPage.class.getSimpleName());
        PageFactory.initElements(driver, this);
        isLoaded();
        helpers.getHar();
    }

    @Override
    protected void load() {}

    @Override
    protected void isLoaded() throws Error {
        helpers.waitUntil(btnAddNewVariable);
    }
}

But wait, we can do better! So, let us also call helpers.setHar(String) and thus we need to refactor the constructor of ExtendedLoadableComponent<T>

public abstract class ExtendedLoadableComponent<T> extends LoadableComponent<T> {

    protected final TestHelpers helpers
    protected WebDriver driver;

    public ExtendedLoadableComponent(Class<T> pageClassToProxy) throws Exception {
        helpers = new TestHelpers();
        driver = helpers.getWebDriver();
        helpers.setHar(pageClassToProxy.getSimpleName());
        PageFactory.initElements(driver, pageClassToProxy);
    }
}

which reduces the first class to

public class VariablesPage extends ExtendedLoadableComponent<VariablesPage> {

    @FindBy(xpath = "//a[@title=\"Define new variable\"]")
    private WebElement btnAddNewVariable;

    public VariablesPage() throws Exception {
        super(VariablesPage.class);
        isLoaded();
        helpers.getHar();
    }

    @Override
    protected void load() {}

    @Override
    protected void isLoaded() throws Error {
        helpers.waitUntil(btnAddNewVariable);
    }
}

Naming

I don't know if the TestHelpers class is your own. If yes, you should think about changing the names for setHar() and getHar() so Mr. Maintainer can evaluate what they do by looking at the names.


Reverse words in a given String I'd like this code to be improved

Input string: codereview is awesome

Output string: awesome is codereview

My approach:

Reverse the entire string
Reverse each word in a string

package com.arun.reversewords;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;

public class ReverseWords {

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

    BufferedReader br = new BufferedReader(new InputStreamReader(System.in));

    System.out.println("Enter the string");
    String s = br.readLine();

    String rev = StringRev(s);

    System.out.println(rev);

}

private static String StringRev(String s) {

    char[] modString = new char[s.length()];

    for (int i = 0; i < s.length(); i++) {
        modString[i] = s.charAt(s.length() - 1 - i);
    }

    s = s.copyValueOf(modString);
    String reverseWord = "";
    String eachWord;
    for (String part : s.split(" ")) {
        eachWord = new StringBuilder(part).reverse().toString();
        reverseWord = reverseWord + eachWord + " ";
    }

    return reverseWord;

  }

}

There are several problems with your approach:

It's strange to reverse the letters, reorder the words, and then reverse the letters again. It's inefficient and unnecessary. You could just reverse the words.
The string you return has a trailing space: "awesome is codereview "
The naming standard is to use camelCase for methods, which "StringRev" violates, and it's not a good name anyway ("reverseWords" would be better)
s for the sentence to reverse is not a good name

It would be more efficient to split by space and then join the words in reverse order. Also keep in mind that String concatenation is not efficient. The efficient way is to use a StringBuilder.

This implementation is more efficient:

public static String reverseWords(String sentence) {
    String[] parts = sentence.split(" ");

    StringBuilder builder = new StringBuilder();
    builder.append(parts[parts.length - 1]);

    for (int i = parts.length - 2; i >= 0; --i) {
        builder.append(" ").append(parts[i]);
    }

    return builder.toString();
}

To verify that the method works correctly, it's good to have unit tests:

@Test
public void testEmptyString() {
    assertEquals("", ReverseWords.reverseWords(""));
}

@Test
public void testWithSingleWord() {
    assertEquals("awesome", ReverseWords.reverseWords("awesome"));
}

@Test
public void testWithTwoWords() {
    assertEquals("is awesome", ReverseWords.reverseWords("awesome is"));
}

@Test
public void testWithSentence() {
    assertEquals("awesome is codereview", ReverseWords.reverseWords("codereview is awesome"));
}

It wasn't part of your specifications, but I would go a bit further and make the solution more robust, by making it work with leading or trailing spaces, and multiple spaces in between words:

public static String reverseWords(String sentence) {
    // trim leading and trailing spaces
    // split on 1 or more spaces between words
    String[] parts = sentence.trim().split("\\s+");
    // ...
}

@Test
public void testWithLeadingSpaces() {
    assertEquals("awesome is codereview", ReverseWords.reverseWords("  codereview is awesome"));
}

@Test
public void testWithMultipleSpaces() {
    assertEquals("awesome is codereview", ReverseWords.reverseWords("codereview   is awesome"));
}

Your program reads a single sentence, reverses the words and prints the output. A BufferedReader can be efficient for processing large inputs, but for such simple uses, a Scanner would be easier and simpler, like this:

public static void main(String[] args) {
    System.out.println("Enter the string");
    Scanner scanner = new Scanner(System.in);
    System.out.println(reverseWords(scanner.nextLine()));
}

With this solution you don't need to declare the method to throw an exception.

For extra user-friendliness, you might want to allow input as command line arguments, and fall back to using System.in only when no arguments were given:

public static void main(String[] args) {
    if (args.length == 0) {
        System.out.println("Enter the string: ");
        Scanner scanner = new Scanner(System.in);
        System.out.println(reverseWords(scanner.nextLine()));
    } else {
        for (String arg : args) {
            System.out.println(reverseWords(arg));
        }
    }
}


Color Rows In DataGridView I am using the below syntax to color code sales persons data.  The issue that I have is that the datagridview returns roughly 12,000 rows so the coloring syntax takes some extensive time to execute.

Is there a way to optimize this code?

private void datagridview1_CellFormatting(object sender, DataGridViewCellFormattingEventArgs e)
{
    foreach (DataGridViewRow row in datagridview1.Rows)
    {
        if (row.Cells["Jake"].Value == System.DBNull.Value && row.Cells["Jones"].Value == System.DBNull.Value && row.Cells["Lati"].Value == System.DBNull.Value && row.Cells["Venitia"].Value == System.DBNull.Value)
        {
            row.DefaultCellStyle.BackColor = Color.Red;
        }
    }
}
private void PopulateIT()
{
    datagridview1.Visible = true;
    lblNoData.Visible = false;

    string connString = @"Connection String";
    string query = @"SELECT * from tempdata";
    SqlConnection conn = new SqlConnection(connString);
    SqlCommand cmd = new SqlCommand(query, conn);
    conn.Open();
    SqlDataAdapter da = new SqlDataAdapter(cmd);
    da.Fill(formattedDT);
    conn.Close();
    da.Dispose();
    datagridview1.DataSource = formattedDT;
    datagridview1.AutoResizeColumns();
}

SqlConnection, SqlCommand and SqlDataAdapter are implementing the IDisposable interface. Enclosing the usage of these objects inside a using statement ensures that the objects will always be proper disposed because the using statement just equals a try...finally where the objects would be disposed in the finally part.

The main problem regarding your performance issue is, that by calling the AutoResizeColumns() method the event will be triggered for each row and each column.
This results for each row and each column that you iterate over every row in the DataGridView.

So if we assume you have 12,000 rows with at least 5 columns .... you can do the math yourself.

The DataGridViewCellFormattingEventArgs has a property RowIndex which allows to access a single row so you should use this.

Summing this up in some code could look like so

private bool shouldFormattingBeDone = false;
private void PopulateIT()
{
    datagridview1.Visible = true;
    lblNoData.Visible = false;

    string connString = @"Connection String";
    string query = @"SELECT * from tempdata";

    using (SqlConnection conn = new SqlConnection(connString))
    using (SqlCommand cmd = new SqlCommand(query, conn))
    using (SqlDataAdapter da = new SqlDataAdapter(cmd))
    {
        conn.Open();
        da.Fill(formattedDT);
    }
    datagridview1.DataSource = formattedDT;

    shouldFormattingBeDone = false;
    datagridview1.AutoResizeColumns();
    shouldFormattingBeDone = true;
}

private void datagridview1_CellFormatting(object sender, DataGridViewCellFormattingEventArgs e)
{
    if (shouldFormattingBeDone == false) { return; }

    DataGridViewRow row = datagridview1.Rows[e.RowIndex];

    if (row.Cells["Jake"].Value == System.DBNull.Value && row.Cells["Jones"].Value == System.DBNull.Value && row.Cells["Lati"].Value == System.DBNull.Value && row.Cells["Venitia"].Value == System.DBNull.Value)
    {
        row.DefaultCellStyle.BackColor = Color.Red;
    }

}

This could be beautified by having an extension method which takes a DataGridViewCell as a parameter like so

public static class DataGridViewExtension
{
    public static bool IsDbNull(this DataGridViewCell cell)
    {
        return cell.Value == DBNull.Value;
    }
}

resulting in  the if condition looking like so

if (row.Cells["Jake"].IsDbNull() && row.Cells["Jones"].IsDbNull() && row.Cells["Lati"].IsDbNull() && row.Cells["Venitia"].IsDbNull())

PS: I can't remember if the connection needs to be open when the constructor of the SqlDataAdapter is called.


Google Sheet with a column of names merged with Google Doc I wrote my first script for google apps, but I am just learning how this all works.  The script works fine, but I am sure there are things that are not done properly.  Any advice is appreciated.

The code creates awards from a Google Sheet where a column of names (last name, first name) is separated and text is replaced on a doc template accordingly.

function createAward() {

var myDoc ='ID';
var mySheet ='ID';
var mySourceFolder ='ID';
var myNewFolder ='ID';
var date = Utilities.formatDate(new Date(), "GMT", "MM-dd-yyyy"); // "yyyy-MM-    dd'T'HH:mm:ss'Z'"

//Data
var ss = SpreadsheetApp.openById(mySheet);
var sheet = SpreadsheetApp.getActiveSpreadsheet().getActiveSheet();
var lastRow = sheet.getLastRow();
var range = sheet.getRange("A:A");
var colNum = 1; //just 1 col
var rowNum = 2; //start at 2 because there is a header
var numRows = 1; //process data 1 row at a time

//Splitting names
for (rowNum = 2; rowNum <= lastRow; rowNum ++){
  var cell = range.getCell(rowNum,colNum).getValues()[0];
  var sCell = cell[0].split(",");
  sheet.getRange(rowNum,colNum +1,1,sCell.length).setValues([sCell]);
}

//Loop to make doc for each student
for (rowNum = 2; rowNum <= lastRow; rowNum ++){
  var dataRange = sheet.getRange(rowNum, 1, numRows, 3)
  var data = dataRange.getValues();

// Data loop --
for (i in data) {
  var row = data[i];
// columns to use for variables
  var fName = row[2];
  var lName = row[1];

// make copy of doc and save as student name.
  var docTemplate = DriveApp.getFileById(myDoc);
  var newFile = docTemplate.makeCopy(fName + lName);
  var newDoc = DocumentApp.openById(newFile.getId());
  DriveApp.getFolderById(myNewFolder).addFile(newFile);
  DriveApp.getFolderById(mySourceFolder).removeFile(newFile);

// replace text
  var doc = DocumentApp.getActiveDocument();
  var body = newDoc.getBody();
  body.replaceText('{first name}', fName);
  body.replaceText('{last name}', lName);
  body.replaceText('{date}' , date)
  }
 }
}

I realized I didn't need all those loops so I simplified things to 1 loop:

for (rowNum = 2; rowNum <= lastRow; rowNum ++){
//Splitting names
  var cell = range.getCell(rowNum,colNum).getValues()[0];
  var sCell = cell[0].split(", ");
  sheet.getRange(rowNum,colNum +1,1,sCell.length).setValues([sCell]);
  var fName = sCell[1]; // first name
  var lName = sCell[0]; // last name

// make copy of doc and save as student name.
  var docTemplate = DriveApp.getFileById(myDoc);
  var newFile = docTemplate.makeCopy(fName + lName);
  var newDoc = DocumentApp.openById(newFile.getId());
  DriveApp.getFolderById(myNewFolder).addFile(newFile);
  DriveApp.getFolderById(mySourceFolder).removeFile(newFile);

// replace text
  var body = newDoc.getBody();
  body.replaceText('{first name}', fName);
  body.replaceText('{last name}', lName);
  body.replaceText('{date}' , date);
  }


STL-like palindrome checking I just read this article on how to write a standard-like algorithm and wanted to try it myself by writing (as suggested in the article) an algorithm to test if a sequence given by two bidirectional iterators is a palindrome.

Here is my code with two different ways of calling the algorithm (is_palindrome and alt_is_palindrome):

#include <cctype>       // std::tolower
#include <functional>   // std::equal_to
#include <iostream>     // std::cout
#include <iterator>     // std::iterator_traits<T>::value_type
#include <string>
#include <vector>

template<typename Iterator>
using ItVType  = typename std::iterator_traits<Iterator>::value_type;

template<typename Iterator>
using StdEqual= typename std::equal_to<ItVType<Iterator>>;

template<typename BidirectionalIterator, typename Compare=StdEqual<BidirectionalIterator>>
bool is_palindrome(BidirectionalIterator first, BidirectionalIterator last, Compare comp=Compare())
{
    for(; first != last && first != --last; ++first)
        if(not comp(*first,*last))
            return false;

    return true;
}

template<typename BidirectionalIterator, typename Compare>
bool alt_is_palindrome(BidirectionalIterator first, BidirectionalIterator last, Compare comp)
{
    for(; first != last && first != --last; ++first)
        if(not comp(*first,*last))
            return false;

    return true;
}

template<typename BidirectionalIterator>
inline bool alt_is_palindrome(BidirectionalIterator first, BidirectionalIterator last)
{
    return alt_is_palindrome(first, last, StdEqual<BidirectionalIterator>());
}

int main()
{
    std::vector<std::string> vs = { "", "palindrome", "abba", "abcBA"};

    std::cout << "case sensitive:" << std::endl;
    for(auto const& s : vs)
        std::cout << s << " is "
            << ( is_palindrome(begin(s),end(s)) ? "" : "not " )
            << "a palindrome." << std::endl;
    std::cout << std::endl;

    auto my_comp = [](auto const& lhs, auto const& rhs)
    {
        return tolower(lhs) == std::tolower(rhs);
    };

    std::cout << "case insensitive:" << std::endl;
    for(auto const& s : vs)
        std::cout << s << " is "
            << ( is_palindrome(begin(s),end(s), my_comp) ? "" : "not " )
            << "a palindrome." << std::endl;
}

So first: I like the style of is_palindrome better with the default arguments, but I haven't seen it used in that way before (maybe I just haven't seen enough code...).
Is there a preferred way to declare this?
If so, is it 'just' a convention or are there actual benefits (performance, security,...) to it?

The second thing I am not sure about are my range-based for-loops and the lambda function int the main function.
As you see I took their respective variables by auto const&. I have sometimes seen people take them as auto&&, but I am not quite sure about he implications of this. Is this way preferred?

If you note anything else that isn't quite right or nice (e.g. the kind of type aliases I use), please let me know, too.

Nothing wrong with these:

template<typename Iterator>
using ItVType  = typename std::iterator_traits<Iterator>::value_type;

template<typename Iterator>
using StdEqual= typename std::equal_to<ItVType<Iterator>>;

But I find them a bit disturbing being so unbound (they are in the global namespace). Maybe just a feeling. I personally would put them in an anonymous namespace.

I also prefer

template<typename BidirectionalIterator,
         typename Compare=StdEqual<BidirectionalIterator>>
bool is_palindrome(BidirectionalIterator first,
                   BidirectionalIterator last,
                   Compare               comp=Compare())

Over

template<typename BidirectionalIterator,
         typename Compare>
bool alt_is_palindrome(BidirectionalIterator first,
                       BidirectionalIterator last,
                       Compare               comp)

template<typename BidirectionalIterator>
inline bool alt_is_palindrome(BidirectionalIterator first,
                              BidirectionalIterator last)

Putting it all in one function does not reduce readability.
But the huge template type names do bother me slightly. Template names are usually short (because they are not real type).

template<typename I,typename Compare=StdEqual<I>>
  // requires BindirectionalIterator<I>()
bool is_palindrome(I first, I last,Compare comp=Compare())

I change my mind this is fine.

for(; first != last && first != --last; ++first)
    if(not comp(*first,*last))
        return false;

But you may want to look at some algorithms.

return std::equal(first, last, std::reverse_iterator(last), comp);

Admittedly I now compare twice as many elements (so not an improvement in this case (informational only)). But if you are using random access elements you can reduce this by calculating the mid point manually.

auto dist    = std::distance(first, last);
auto halfway = first;
std::advance(halfway, dist/2);
return std::equal(first, halfway, std::reverse_iterator(last), comp);

Looks like this should not compile:

 is_palindrome(begin(s),end(s))

That should be std::begin() and std::end() so it looks like you have a hidden using namespace std; somewhere.

If you are going to declare your lambda like this:

auto my_comp = [](auto const& lhs, auto const& rhs)
{
    return tolower(lhs) == std::tolower(rhs);
};

You may as well put it in a separate function. The point of lambdas is you can see them in place.

is_palindrome(begin(s),end(s),
              [](auto const& lhs, auto const& rhs) {return tolower(lhs) == std::tolower(rhs);}
             );


Implementation of Merge Sort in C++ I'd originally not copied the result vector to vec which I figured and corrected. Iis there a better way to implement the merge sort other than the approach I've taken?

template<typename T> void merge(std::vector<T> &vec, int left, int mid, int right )
{
    int i=left;
    int j=mid+1;
    int k=0;
    int size = (right - left) +1;
    std::vector<T> result(size);

    while(i <= mid && j <= right)  result[k++] = (vec[i] < vec[j])? vec[i++] : vec[j++];
    while(i <= mid)                result[k++] = vec[i++];
    while(j <= right)              result[k++] = vec[j++];

    for(k=0; k < size; k++)
    {
        vec[left+k] = result[k];
    }
}

template<typename T> void mergeSort(std::vector<T> &vec, int left, int right)
{
    // Base Case --- Left is greater than right, then don't execute.

    if (left<right){

        // get mid point.
        int mid = left + (right-left)/2 ;
        // recursive merge sort until array(half) is 1 in length.
        mergeSort(vec, left, mid);
        mergeSort(vec, mid+1, right);
        // merge both arrays.
        merge(vec, left, mid, right);
    }
}

main():

int main()
{
    vector<int> v{14,3,2,5,10};
    //bubbleSort(v);
    //selectionSort(v);
    mergeSort(v, 0, v.size()-1);
    printVec(v);

    return 0;
}

I'm trying to figure out space optimization considerations. Currently, the space is \$O(N)\$ (since I'm creating a resultant array in merge which creates a max size of N? Please correct me if I'm wrong here.)

Instability

while(i <= mid && j <= right)  result[k++] = (vec[i] < vec[j])? vec[i++] : vec[j++];

Whenever vec[i] == vec[j], the above excerpt from your code will favour the element from the right chunk, which reorders equal elements. Instead you should say:

while(i <= mid && j <= right)  result[k++] = (vec[i] <= vec[j])? vec[i++] : vec[j++];

or even:

while(i <= mid && j <= right)  result[k++] = (vec[j] < vec[i])? vec[j++] : vec[i++];

Misc 1

while(i <= mid)                result[k++] = vec[i++];
while(j <= right)              result[k++] = vec[j++];

You could use std::copy from algorithm header file to do the above; it might be (or might not) be optimized for speed.

Misc 2

Actually, there is std::merge in algorithm; why not use it?

API

At this point, your implementation is restricted to std::vector. It is not hard to make it accept any sequence:

template<typename RandIt1, typename RandIt2>
void mergeSort(RandIt1 source_begin,
               RandIt1 source_end,
               RandIt2 target_begin,
               RandIt2 target_end)
{
    auto range_length = std::distance(source_begin, source_end);

    if (range_length < 2)
    {
        return;
    }

    auto left_subrange_length = range_length >> 1;

    mergeSort(target_begin,
              target_begin + left_subrange_length,
              source_begin,
              source_begin + left_subrange_length);

    mergeSort(target_begin + left_subrange_length,
              target_end,
              source_begin + left_subrange_length,
              source_end);

    std::merge(source_begin,
               source_begin + left_subrange_length,
               source_begin + left_subrange_length,
               source_end,
               target_begin);
}

template<typename RandIt>
void mergeSort(RandIt begin, RandIt end)
{
    auto range_length = std::distance(begin, end);

    if (range_length < 2)
    {
        return;
    }

    using value_type = typename std::iterator_traits<RandIt>::value_type;
    std::vector<value_type> aux(begin, end);
    mergeSort(aux.begin(), aux.end(), begin, end);
}

template<typename T>
void mergeSort(std::vector<T>& vec)
{
    mergeSort(vec.begin(), vec.end());
}

Hope that helps.

Peformance

In this Gist you can get everything needed for a performance demonstration. I get the following figures:

OP mergeSort in 3511 milliseconds.
cr mergeSort in 1653 milliseconds.
stable_sort in 1332 milliseconds.
Algorithms agree: true

Space consideration

At any given instant, you have \$\Theta(N)\$ worth memory allocated. However, if you count all std::vector<T> result(size);, you will get \$\Theta(N \log N)\$ worth memory allocated.

You can do better. You can allocate a vector that is of the same length and content as the input vector. Then, you merge from one vector to another; and at the next recursion level you swap their roles and so on. That way, you can eliminate

for(k=0; k < size; k++)
{
    vec[left+k] = result[k];
}

in the merging function.

Suppose also that we are sorting 8 elements. At the very highest recursion level you allocate a vector of 8 elements. After that you visit both left and right subranges of length 4 elements each. That's 8 elements in vector(s) more. You continue the same argument until you reach the bottom recursion level. Since each level allocates \$N = 8\$ worth memory, and there is \$\Theta(\log N)\$ levels total, you get \$\Theta(N \log N)\$.


Unix-to-Unix Encoding (Uuencoding) Uuencoding is historically used to encode emails. Instructions for creating a Uuencoder are:

Start with 3 bytes from the source, 24 bits in total.
Split into 4 6-bit groupings, each representing a value in the range 0 to 63: bits (00-05), (06-11), (12-17) and (18-23).
Add 32 to each of the values. With the addition of 32 this means that the possible results can be between 32 (" " space) and 95 ("_" underline). 96 ("`" grave accent) as the "special character" is a logical extension of this range.
Output the ASCII equivalent of these numbers.

import platform

def dec_to_bin(int):
    return bin(int).replace('0b', '')

def uuencode(string, filename):
    if platform.system() == 'Windows':
        mode = '644'
    elif platform.system() == 'Linux':
        mode = '744'

    trail = 'begin ' + mode + ' ' + filename + '\n'
    string_values = []
    char_list = list(string)
    bytes = ''

    for char in char_list:
        string_values.append(ord(char))

    for ascii in string_values:
        bytes += dec_to_bin(ascii)

    three_byte = [bytes[p:p+24] for p in range(0, len(bytes), 24)]
    if len(three_byte[-1]) < 24:
        three_byte[-1] += (24 - len(three_byte[-1])) * '0'

    four_six_bits = [three_byte[n][m:m+6] for n in range(len(three_byte)) for m in range(0, 24, 6)]
    four_six_bits = [four_six_bits[k:k+4] for k in range(0, len(four_six_bits), 4)]

    decimal_list = []
    for x in range(len(four_six_bits)):
        for z in range(4):
            decimal_list.append(int(four_six_bits[x][z], 2))

    for index, decimal in enumerate(decimal_list):
        decimal_list[index] += 32

    encoded_chars = [chr(decimal_list[o]) for o in range(len(decimal_list))]

    length_char = chr(len(encoded_chars) + 32)
    trail += length_char

    for newchar in encoded_chars:
        trail += newchar

    trail += '\n' + '`' + '\n' + 'end' + '\n'

    return trail

This is somewhat unstructured, but here are a few recommendations:

Be careful when shadowing built-ins

In dec_to_bin(), the int parameter shadows the Python core int type keyword. This works fine, but it's usually clearer to avoid redefining builtins when you can. Maybe call it num.

bytes is also a builtin. It's ok to redefine them, just be aware.

Variables not always defined

if platform.system() == 'Windows':
    mode = '644'
elif platform.system() == 'Linux':
    mode = '744'

trail = 'begin ' + mode + ' ' + filename + '\n'

What happens if platform.system() is neither Windows or Linux? (For example: on Mac it returns 'Darwin'.) mode will be undefined, raising UnboundLocalError.

Unnecessary conversion to list

char_list = list(string)
...
for char in char_list: ...

Strings are iterable just like lists, so for char in string: works fine.

In fact, consider removing string_values and char_list entirely. You can condense them into:

bytes = ''.join(dec_to_bin(ord(char)) for char in string)

Unused variable

A common convention is to use _ to represent unused vars:

for index, _ in enumerate(decimal_list):
    decimal_list[index] += 32

This shows that you are deliberately ignoring the second value from enumerate.


Merging, sorting and limit Map streams using Java 8 I have two maps map1 and map2 . I want to merge both maps and then sort in desc order and get top 5. In case of duplicate keys in merge I need to sum the values. I have the following code that works:

Map<String, Long> topFive = (Stream.concat(map1.entrySet().stream(), map2.entrySet().stream())
        .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, Long::sum)))
        .entrySet().stream().sorted(Map.Entry.comparingByValue(Comparator.reverseOrder()))
        .limit(5).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue,
            (v1,v2) -> v1,
            LinkedHashMap::new));

But I would like to know if there is a better solution.

The question for a "better" solution is bound to be quite opinion-based. As far as the actual code goes, I think you are quite close to the optimum. As for readability... well, Roland already shared his initial thoughts and I totally agree. This is bound to be a nightmare for the poor maintenance programmer.

Thus: a suggestion which is nothing new, but just a little rearangement:

private Map<String, Long> getHistogramTopFive(Map<String, Long> map1, Map<String, Long> map2) {
    Map<String, Long> mergedMap = (Stream.concat(map1.entrySet().stream(), map2.entrySet().stream())
        .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, Long::sum)));

    Map<String, Long> topFive = mergedMap.entrySet().stream()
        .sorted(Map.Entry.comparingByValue(Comparator.reverseOrder()))
        .limit(5)
        .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue,
                (v1,v2) -> v1,
                LinkedHashMap::new));

    return topFive;
}

By putting it into a method and naming this method, you document your business intention. Furthermore, by breaking up the long command chain with a new mergedMap variable in between, the code gains much in readability.

The additional line-breaks in the second part are what I consider best practices in streaming API: each new operation on a new line.


Getting student demographics report with ActiveRecord I have two active records StudentDemographics and StudentWeeklyReport both having has_many relation like this:

class StudentDemographics < ActiveRecord::Base
  has_many :student_weekly_reports, :foreign_key => :student_id
end

I have to check marks for each student in the last fifth week seminar with the latest one. If the result is true, student should be active, else inactive. I have the following code. Here, I am repeating the loop with each date. @distinct is an array of dates.

def active_learner

  safe = []

  @active = []

  @inactive = []

  @distinct = StudentDemographics.select(:date).map(&:date).uniq.sort

  @students = StudentDemographics.includes(:student_weekly_reports).select("student_id,date")

@distinct.each_with_index  do |date,i|

 if i < 4

     @count = StudentDemographics.where("date <= ?", date).count

     @active[i] = @count

     @inactive[i] = 0

 else

     sum = safe.length

     active = inactive = 0

     (@students - safe).each do |student|

        next if student.date > date

        @stu = student.student_weekly_reports.last(5)

            if @stu.length > 4

                    if @stu[4].golden_eggs > @stu[0].golden_eggs

                      safe << student

                      active += 1

                    else

                      inactive += 1

                    end

            else

                    safe << student

                    active += 1

            end

      end

      @active[i] = active + sum

      @inactive[i] = inactive

 end

end
end

The performance is not good. It's taking more than 3 secs of time. My MySQL database has 13600 in the StudentWeeklyReports table and 2000 in the StudentDemographics table. Can anyone suggest how I can optimize this?

I'd suggest a couple of things:

the for i in 0...@distinct.length takes your array's length and uses it to construct a range which it turns into an array again. Not necessarily time consuming but unnecessary and hard to read. Ruby gives you each_with_index which can do it in a nicer, more concise way.
favoring working with IDs should speed things up because no Active Record objects need to be created and filled and it's quite enough for your calculations. One important tool for that is pluck.
I also removed several local variables as you only use them for tallies and that's, again, not really necessary. But no real speedup here, either.

TL;DR: here's the re-factored version with calculations in ruby.

safe_ids = safe.map(&:student_id)
@distinct.each_with_index do |date, i|
  @active[i] = safe_ids.length
  @inactive[i] = 0
  @student_ids = StudentDemographics.where("id not in (?)", safe_ids).where("date <= ?", date).pluck("student_id")
  @student_ids.each do |student_id|
    @stu = StudentWeeklyReport.where(:student_id => student_id).last(5).pluck(:golden_eggs)
    if @stu.length <= 4 || @stu[4] > @stu[0]
      safe << student_id
      @active[i] += 1
    else
      @inactive[i] += 1
    end
  end
end

You could also try and have the whole calculation done by the database, by using group_by and having in a fairly complicated but probably very fast query.

But I'll leave it to the astute reader to figure that one out (mainly since I couldn't manage to get it right in time).


Read/Write a TXT file with timestamp I have write this code that read a txt file and re-write it whith the timestamp sorted.

This is the result (top image is input, bottom is output)

MY QUESTION IS:
Anyone do you have a suggestion to optimize this code? Because this text is a little example,my code would work with txt more longer than it. (For example 50K-60K line)

Regards

static void Main(string[] args)
{

    string[] str;

    //Define regex string for MEX ETCS
    string messageETCS = @"(\d){4}-(\d){2}-(\d){2}.*(?:\r?\n(?!\d{4}-\d{2}-\d).*)*";
    Regex regMEXETCS = new Regex(messageETCS);

    string regexDate = @"\d{4}-\d{2}-\d{2}";
    Regex regDateETCS = new Regex(regexDate);

    // Open the text file using a stream reader.
    using (StreamReader sr = new StreamReader(@"C:\input.txt"))
    {
        // Read the stream to a string, and write the string to the console.
        String logContent = sr.ReadToEnd();

        //run regex
        List<String> MasterList = new List<String>(logContent.Length);
        Match matchResults = regMEXETCS.Match(logContent);
        Match onlyDate = regDateETCS.Match(logContent);
        str = Regex.Split(logContent, @"\d{4}-\d{2}-\d{2}", RegexOptions.None);

        while (matchResults.Success)
        {

            for (int j = 1; j < str.Length;j++ ) {

            string t = str[j].TrimEnd();
            MasterList.Add(t);
            matchResults = matchResults.NextMatch();
            }
        }

        String date1 = onlyDate.ToString();
        MasterList.Sort();
        using (TextWriter tw = new StreamWriter(@"C:\output.txt"))
        {
            for (int i = 0; i < MasterList.Count; i++)
            {
                tw.WriteLine(date1 + MasterList[i]);
            }
        }
    }
}

I'm quite surprised that the messageETCS regex actually works.
Probably the best and fast way it's to use a parser (ex: https://github.com/picoe/Eto.Parse).

To improve performances, read and process the file chunk by chunck, or line by line if those are lines