Finding divisors of a number This code finds all the divisors of a given number.  Can it be shortened?

import java.util.Scanner;

public class PrimeNum2{
    public static void main(String args[]){
        Scanner x=new Scanner(System.in);
        System.out.print("Enter the number :  ");
        long y=x.nextInt(),i;
        System.out.print("Divisors of "+y+" = 1 , ");

        for( i=2;i<y;i++){
            long z=y%i;
            if(z!=0)continue;
                System.out.print(i+" , ");

        }System.out.println(y);
    }
}

This code could do with some editing...

First of all is the spacing. It is absolutely horrible (we will fix that after the edits).

Also, the naming is horrible. Scanner x could be scanner and y could be num. As for z, it is completely unnecessary:

for (i = 2; i < y; i++) {
    long z = y % i;
    if (z != 0)
        continue;
    System.out.print(i + " , ");
}

Becomes:

for (i = 2; i < y; i++) {
    if (y % i != 0)
        continue;
    System.out.print(i + " , ");
}

The program can do without the continue statement:

for (i = 2; i < y; i++) {
    if (y % i == 0)
        System.out.print(i + " , ");
}

It's also a good idea to put braces around statements in an if statement, even when there is only one:

for (i = 2; i < y; i++) {
    if (y % i == 0) {
        System.out.print(i + " , ");
    }
}

You are also wasting time going through for loops doing nothing. After all, num's largest factor before itself possible is num / 2, which makes it more efficient doing it like this:

for (i = 2; i <= num / 2; i++) {
    if (num % i == 0) {
        System.out.print(i + " , ");
    }
}

I also noticed:

 public static void main(String args[])

It is better to put [] at the type (String):

public static void main(String[] args)

But the main problem is that you have a memory leak. It could be solved by closing the Scanner:

scanner.close();

Final code:

Your final code will look like this:

public class PrimeNum2 {
    public static void main(String args[]) {
        Scanner scanner = new Scanner(System.in);
        System.out.print("Enter the number :  ");
        long num = scanner.nextInt(), i;
        System.out.print("Divisors of " + num + " = 1 , ");
        for (i = 2; i <= num / 2; i++) {
            if (num % i == 0) {
                System.out.print(i + " , ");
            }
        }
        System.out.println(num);
        scanner.close();
    }
}


Getting columns from lines of uneven length I am given pasted data from a table, so I have spaces as delimiters and in some of the fields I don't care about. I want the first field and the last three fields, and got them using this code:

testdata = """8/5/15 stuffidontneed custid locid 55.00
8/9/15 stuff i really dont need with extra spaces custid otherlocid 79.00"""
rows = testdata.split('\n')
tupls = [row.split(' ') for row in rows]
dates = [tupl[0] for tupl in tupls]
custids, locids, amounts = ([tupl[i] for tupl in tupls] for i in range (-3, 0))
print(dates, custids, locids, amounts)
# ['8/5/15', '8/9/15'] ['custid', 'custid'] ['locid', 'otherlocid'] ['55.00', '79.00']

I just thought there might be a more elegant way to do things, maybe capturing the data in the middle as a single field.

Edit: I have attempted to add delimiters using re.finditer, but I can't replace the matches easily.

After you have tupls:

data = [(t[0], t[-3], t[-2], t[-1] for t in tupls] # Or use range...
print(list(zip(*data))

Gives:

[('8/5/15', '8/9/15'), ('custid', 'custid'), ('locid', 'otherlocid'), ('55.00', '79.00')]

So this:

testdata = """8/5/15 stuffidontneed custid locid 55.00
8/9/15 stuff i really dont need with extra spaces custid otherlocid 79.00"""
rows = testdata.split('\n')
tupls = [row.split(' ') for row in rows]
dates = [tupl[0] for tupl in tupls]
custids, locids, amounts = ([tupl[i] for tupl in tupls] for i in range (-3, 0))
print(dates, custids, locids, amounts)

Becomes:

testdata = """8/5/15 stuffidontneed custid locid 55.00
8/9/15 stuff i really dont need with extra spaces custid otherlocid 79.00"""
rows = testdata.split('\n')
tupls = [row.split(' ') for row in rows]
data = [(t[0], t[-3], t[-2], t[-1] for t in tupls] # Or use range...
print(list(zip(*data))


Optimization in overcoming TLE in interviewStreet problem I have been trying to get the problem Quadrant Queries correct for a while now. Although my approach is an optimal one, I am getting a TLE on 3 test cases. Please help me optimize my approach so that I can pass all test cases for the problem.

#include <iostream>
#include <cstdlib>
#include <cstring>
#include <algorithm>
#include <vector>
#include <utility>

using namespace std;

typedef pair <int,int> pii;

void performCquery(int i,int j, const vector< pii > &pointList)
{
    i = i-1;
    j = j-1;
    int q1 = 0;
    int q2 = 0;
    int q3 = 0;
    int q4 = 0;

    for (int k = i; k <=j ; k++) {
        if ( (pointList[k].first > 0 ) && ( pointList[k].second > 0) ) {
            q1++;
        }
        else if ( (pointList[k].first < 0) && ( pointList[k].second > 0) ) {
            q2++;
        }
        else if  ( ( pointList[k].first < 0) && (pointList[k].second < 0 ) ) {
            q3++;
        }
        else if ( ( pointList[k].first > 0) && (pointList[k].second < 0 ) ) {
            q4++;
        }
    }

    cout << q1 << " " << q2 << " " << q3 << " " << q4 << endl;
}

void performXYquery(char qType, int i, int j, vector< pii > &pointList)
{
        i = i-1;
        j = j-1;
    for ( int k = i; k <= j ;k++) {
        pii pnt = pointList[k];
        if ( qType == 'X') {
            pointList[k].second = -pointList[k].second;
        }
        else if (qType == 'Y') {
            pointList[k].first = -pointList[k].first;
        }
    }
}

int main(int argc, char* argv[])
{
    int numOfPoints;
    cin >> numOfPoints;

    vector < pii > pointList;

    for (int i = 0; i < numOfPoints ; i++) {
        int x,y;
        cin >> x >> y;
        pii temp(x,y);
        pointList.push_back(temp);
    }

    int numOfQueries;
    cin >> numOfQueries;

    for (int i = 0; i < numOfQueries; i++) {
        char qName;
        int startIndex;
        int endIndex;
        cin >> qName >> startIndex >> endIndex;
        if (qName == 'C') {
            performCquery(startIndex,endIndex, pointList);
        }
        else if (qName == 'X' || qName == 'Y') {
            performXYquery(qName, startIndex,endIndex, pointList);
        }
        else {
            cerr << "Error: This line should not be printed" <<endl;
        }
    }

    return 0;
}

Algorithmic optimizations.
As you read the values in work out what quadrant the point belongs to and keep a running total for each quadrant (not adjustment needed on X/Y query). Then you do not need to re-calculate everything every time there is a 'C' request.
As it stands complexity for each C request of O(n). If you keep a running total then it is merely o(1). Think of the situation where you have a 1,000,000 points and you do a 1,000,000 C requests. The code would basically do 1,000,000 * 1,000,000 operations rather than just 1,000,000.
Style
Please stop using this:
using namespace std;

For simple ten line programs it makes things simpler. But once you get past that stage it makes things exceedingly more complex. By causing name clashing problems. It is preferable to prefix stuff in the standard namespace with std:: (why do you think they kept it so short).
A personal opinion. So feel free to ignore. But to distinguish types and objects. I nae types with a leading capitol letter. A lower case indicates an object (or reference).
typedef pair <int,int> pii;

What happens below if any point is on the line (first or second == 0)?
    if ( (pointList[k].first > 0 ) && ( pointList[k].second > 0) ) {
        q1++;
    }
    else if ( (pointList[k].first < 0) && ( pointList[k].second > 0) ) {
        q2++;
    }
    else if  ( ( pointList[k].first < 0) && (pointList[k].second < 0 ) ) {
        q3++;
    }
    else if ( ( pointList[k].first > 0) && (pointList[k].second < 0 ) ) {
        q4++;
    }
}

Here:
void performXYquery(char qType, int i, int j, vector< pii > &pointList)

For readability I would just make this two separate functions performXquery() and performYquery(). It make the code more readable and removes the need for an if statement in the middle of a tight loop.
There is a standard utility function for creating std::pair<> called make_pair.
    pii temp(x,y);
    pointList.push_back(temp);

    // can be written as:
    pointList.push_back(std::make_pair(x, y));

Since qName is a char you can use a switch statement here:
    if (qName == 'C') {
        performCquery(startIndex,endIndex, pointList);
    }
    else if (qName == 'X' || qName == 'Y') {
        performXYquery(qName, startIndex,endIndex, pointList);
    }

    // can be written as:
    switch(qName)
    {
        case 'C': performCquery(startIndex,endIndex, pointList);break;
        case 'X': performXquery(startIndex,endIndex, pointList);break;
        case 'Y': performYquery(startIndex,endIndex, pointList);break;
        default: /* Error Msg */
    }

No need to return 0 from main() in C++. If no explicit return is placed then an automatic return 0 is planted.
Algorithmic optimizations.
As you read the values in work out what quadrant the point belongs to and keep a running total for each quadrant (not adjustment needed on X/Y query). Then you do not need to re-calculate everything every time there is a 'C' request.
As it stands complexity for each C request of O(n). If you keep a running total then it is merely o(1). Think of the situation where you have a 1,000,000 points and you do a 1,000,000 C requests. The code would basically do 1,000,000 * 1,000,000 operations rather than just 1,000,000.
Macro optimizations
Only do these to make the code more readable. Usually the compiler already handles this kind of optimization.
Assume we have an average distribution of points in 4 quadrants:
100 pts                 // cmp == conditional test like > or <
25 in q1 use 2   cmp    // must pass both to enter
25 in q2 use 3.5 cmp    // Note: average (12.5 will do 3 cmp 12.5 will do 4 cmp)
25 in q3 use 5   cmp    //       to enter the code section that will do q2++
25 in q4 use 6.5 cmp

Total: 425 cmp

If we change the code so you include values on the line (ie first or second == 0) in an appropriate quadrant. You don't need the last if statement which reduces the number of comparisons so that:
100 pts                 // cmp == conditional test like > or <
25 in q1 use 2   cmp    // must pass both to enter
25 in q2 use 3.5 cmp
25 in q3 use 5   cmp
25 in q4 use 4.5 cmp

Total: 375 cmp

If we re-range the tests:
if (first >= 0)
{
    if (second >= 0)  ++q1;
    else              ++q4;
}
else // first = 0
{
    if (second >= 0)  ++q2;
    else              ++q3;
}

In this case there are always 2 cmp:
100 nodes * 2 cmp = 200 cmp
Approximately a 100% reduction in the number of cmp.
Note: this does not mean a doubling of speed (maybe a slight improvement on average).
      Assuming the compiler has not already worked this out.

But in my opinion makes the code more readable.


Simple temperature converter Below is my first attempt at programming a Celsius to Fahrenheit converter in C# winforms. I'm looking for tips and advice on improving my style and what I can do to make the code more efficent/proper, or is it good as is? I apologize ahead of time if i've forgotten any details and would be happy to provide more upon request.

namespace TemperatureConverter
{
  public partial class Form1 : Form
  {
    public Form1()
    {
        InitializeComponent();
        // Initalize the handler to allow use of only control/number characters.
        this.txtCelsius.KeyPress += new KeyPressEventHandler(txtCelsius_KeyPress);
        // Limits user's inpute to 5 characters max.
        txtCelsius.MaxLength = 5;
        // Prevents the user from resizing window.
        this.FormBorderStyle = FormBorderStyle.FixedDialog;
    }
    // Main function. Converts the input into Fahrenheit and outputs to the targeted label.
    private void btnConvert_Click(object sender, EventArgs e)
    {
        double C;
        double F;

        C = double.Parse(txtCelsius.Text);
        F = C * 9 / 5 + 32;
        lblFahrenheit.Text = F.ToString();
    }
    // Prevents user from entering anything other than Number and Control characters.
    private void txtCelsius_KeyPress(object sender, KeyPressEventArgs e)
    {
        if (!Char.IsControl(e.KeyChar) && !Char.IsNumber(e.KeyChar))
        {
            e.Handled = true;
        }
    }
  }
}

The main change is separation of logic and User Interface. Firstly the calculation methods should ideally not reside in UI methods like event handlers.

Secondly the calculations should really be in it's own class, (but this application is just small enough that it is not ENTIRELY necessary).

in programming we hate Magic Numbers. If you can, move any numbers littered around your code and name them something useful.

so:

As 9/5 + 32 is equal to 1.8 (+32) and that number represents the scale difference, if we declare that at the top of our code like

double FAHRENEHEIT_SCALE_DIFFERENCE = 1.8;
double FREEZING_POINT_OF_WATER = 32;

Depending on how much you have covered thus far you should then move the actual conversion to it's own method.

(because if you come back to this code in 12-18months will you be able to remember what it does? What if it quadruples in size with more features?

So:

I would suggest something along the lines of

public double convertCelsiusToFahrenheit(double celsius)
{
  return celsius * FAHRENEHEIT_SCALE_DIFFERENCE + FREEZING_POINT_OF_WATER;
}

Next,
you can tidy the button click.

private void btnConvert_Click(object sender, EventArgs e)
{
    double celsius = double.Parse(txtCelsius.Text);
    lblFahrenheit.Text = convertCelsiusToFahrenheit(celsius);
}

Finally the golden change is the separation of the logic from the interface.

Now that you have a simple method that is separate from the actual screen; you could move it, and all similar methods into it's own TemperatureConverter class, or something similar.


Merge two DataTables by adding corresponding entries I've been looking over internet for a method of combining 2 datatables with exactly the same cells with numbers to add them together, Merge method never help because it just appended the second table. The task was later accomplished by my hand-written method:

static DataTable CombineDT34(DataTable dt3, DataTable dt4) {
        var combinedDT34 = dt3;
        for (var r3 = 0; r3 < dt3.Rows.Count; r3++) {

            for(var c3 = 1; c3 < dt3.Columns.Count; c3++) {

                dt3.Rows[r3][c3] =
                    (Convert.ToInt32(dt3.Rows[r3][c3]) +
                    Convert.ToInt32(dt4.Rows[r3][c3]));
            }
        }

        return dt3;
    }

Is there are a way to do this better?

You are not testing for same size.

You don't use combinedDT34.  You are changing an input which is not a good practice.  I am not sure if dt3 will be passed by reference.

You are starting column on 1.

static DataTable CombineDT34(DataTable dt3, DataTable dt4)
{
    if(dt3.Rows.Count != dt4.Rows.Count)
        throw new ArgumentOutOfRangeException);
    //...

    for (var r3 = 0; r3 < dt3.Rows.Count; r3++)
    {
        for(var c3 = 0; c3 < dt3.Columns.Count; c3++)
        {
            dt3.Rows[r3][c3] = ( Convert.ToInt32(dt3.Rows[r3][c3]) +
                                 Convert.ToInt32(dt4.Rows[r3][c3]) );
        }
    }
    return dt3;
}


Is this too long for a web request? This is from a C# library I'm writing that checks for updates through a PHP script.

public bool CheckForUpdate(Request Request)
{
    HttpWebRequest httpRequest = (HttpWebRequest)HttpWebRequest.Create(UpdateServerAddress + "http://{0}/update.php?{1}" + Request.ToString());
    httpRequest.UserAgent = "KMUpdaterClient/" + Assembly.GetExecutingAssembly().GetName().Version.ToString();
    httpRequest.Accept = "text/plain";
    HttpWebResponse response = (HttpWebResponse)httpRequest.GetResponse();
    if (response.StatusCode == HttpStatusCode.OK)
    {
        string responseString = String.Empty;
        using(StreamReader sr = new StreamReader(response.GetResponseStream()))
        {
            responseString = sr.ReadToEnd();
        }
        response.Close();
        switch (responseString.Substring(0, 3))
        {
            case "UTD":
                //Application is up to date
                return false;
            case "AVL":
                return true;
            case "ERR":
                throw new ApplicationException("An error occured on the server: " + responseString.Substring(4));
            default:
                //Unexpected
                throw new ApplicationException("An unexpected response was recieved by the server: " + responseString);
        }
    }
    else
    {
        switch (response.StatusCode)
        {
            //Generated by web app
            case HttpStatusCode.BadRequest:
                throw new ArgumentException(String.Format("The Request passed to CheckForUpdate was incomplete: AppName:{1}; Channel:{2}; Version:{3}", Request.AppName, Request.Channel, Request.Version));
            case HttpStatusCode.Unauthorized:
                throw new UnauthorizedAccessException("The serial number for this application was invalid.");
            case HttpStatusCode.ServiceUnavailable:
                throw new AppDomainUnloadedException("The web service is currently unavailable.");
            //Common server errors
            case HttpStatusCode.NotFound:
                throw new FileNotFoundException("The updater server page could not be accessed.");
            case HttpStatusCode.InternalServerError:
                throw new WebException("The web server encountered an error.");
            //Unexpected server errors
            default:
                throw new ApplicationException("An unexpected error occured: " + response.StatusCode);
        }
    }
}

After writing it, I just felt that it was a bit too long. Is this an OK size for a web request? If not, where could I trim it down?

You could use guard statements and extract out the switch(response.StatusCode) block to a method. Furthermore, I'd replace the three-letter magic strings with constants. Their name could come from the comment around the magic string (UP_TO_DATE for "UTD" etc.). It would make the comment(s) unnecessary.

private Exception CreateHttpStatusException(Request Request, HttpWebResponse response) {
        switch (response.StatusCode)
        {
            //Generated by web app
            case HttpStatusCode.BadRequest:
                return new ArgumentException(String.Format("The Request passed to CheckForUpdate was incomplete: AppName:{1}; Channel:{2}; Version:{3}", Request.AppName, Request.Channel, Request.Version));
            case HttpStatusCode.Unauthorized:
                return new UnauthorizedAccessException("The serial number for this application was invalid.");
            case HttpStatusCode.ServiceUnavailable:
                return new AppDomainUnloadedException("The web service is currently unavailable.");
            //Common server errors
            case HttpStatusCode.NotFound:
                return new FileNotFoundException("The updater server page could not be accessed.");
            case HttpStatusCode.InternalServerError:
                return new WebException("The web server encountered an error.");
            //Unexpected server errors
            default:
                return new ApplicationException("An unexpected error occured: " + response.StatusCode);
        }
}

public bool CheckForUpdate(Request Request)
{
    HttpWebRequest httpRequest = (HttpWebRequest)HttpWebRequest.Create(UpdateServerAddress + "http://{0}/update.php?{1}" + Request.ToString());
    httpRequest.UserAgent = "KMUpdaterClient/" + Assembly.GetExecutingAssembly().GetName().Version.ToString();
    httpRequest.Accept = "text/plain";
    HttpWebResponse response = (HttpWebResponse)httpRequest.GetResponse();
    if (response.StatusCode != HttpStatusCode.OK)
    {
        throw CreateHttpStatusException(Request, response);
    }

    string responseString = String.Empty;
    using(StreamReader sr = new StreamReader(response.GetResponseStream()))
    {
        responseString = sr.ReadToEnd();
    }
    response.Close();
    switch (responseString.Substring(0, 3))
    {
        case "UTD":
            //Application is up to date
            return false;
        case "AVL":
            return true;
        case "ERR":
            throw new ApplicationException("An error occured on the server: " + responseString.Substring(4));
        default:
            //Unexpected
            throw new ApplicationException("An unexpected response was recieved by the server: " + responseString);
    }
}


Simple DB API for inserting a record I'm looking to make some improvements to a deliberately crude API written in PHP, it's designed to handle simple requests and talk to the DB.

I have the following DB class:

class DB
{
    public function connect()
    {
        $serverName = ".";
        $connectionInfo = array("Database"=>"Demo","UID"=>"sa","PWD"=>"qwe123");
        $connection = sqlsrv_connect( $serverName, $connectionInfo);

        return $connection;
    }

    public function close($connection)
    {
        return sqlsrv_close($connection);
    }

    public function query($connection, $query, $params = null)
    {
        return sqlsrv_query($connection, $query, $params);
    }
}

And then for requests I do:

$db = new DB();
$conn = $db->connect();

$sql = "INSERT INTO Applications (name, friendly_name, description, enabled, visible, icon, video) VALUES (?, ?, ?, ?, ?, ?, ?)";

$params = array($name, $friendly_name, $description, $enabled, $visible, $icon, $video);

$query = $db->query($conn, $sql, $params);

if( $query === false )
{
    $response = array('error' => 'Error');
}
else
{
    $response = array('success' => 'Success');
}

$db->close($conn);

return $response;

I was wondering how I could improve this further and not have to repeat the db connect and creating an instance of the class and close the connection.

I know I could and should be using PDO, but for this example I want to keep it old school.

When you are using a class you can utilize a private/protected class property. As said you know about PDO and since I am not very familiar with the old school sqlsrv_* functions I have used PDO in the examples. An example could be.

class Database {

    /*
     * Having a private/protected property ensures that only
     * the class itself can change/removed the connection. This way calling
     * code cannot mess something up regarding the connection.
     */
    private $pdo;

    public function __construct($dsn, $username, $password, array $options)
    {
        $this->pdo = new PDO($dsn, $username, $password, $options);
    }

    public function __destruct()
    {
        $this->close();
    }

    /**
     * This is an utility method to expose the PDO instance. This
     * enables calling code to use the full toolset of PDO.
     *
     * @return PDO
     */
    public function pdo()
    {
        return $this->pdo;
    }

    public function query($sql)
    {
        /*
         * I do not enclose this inside a try-catch block
         * as only the calling code can decide if the application
         * can recover from the exception state.
         */
        return $this->pdo->query($sql);
    }

    public function close()
    {
        /*
         * Since PDO is an object closing a connection
         * is done by assigning the value NULL.
         */
        $this->pdo = null;
    }

}

If you look at the Database::close() method you can see that no parameters are required. We already have the property storing the current connection available. I am also using a __destruct() method to ensure the connection is closed as soon as the object loses all references. This frees up server resources earlier than if the connection was closed when the script ended.

You can also store the connection details/credentials inside the class an lazy load the connection only when it is actually required. There are plenty of tutorials around the web about this.

Hope this can help, happy coding!


A CRUD blog with a basic auth system made with the MERN stack I would like some feedback on things that maybe could've been written a lot better. I used redux for handling state.

GitHub repo

Demo

This blog project is meant to create, read, update, and delete posts. Those are the major functions. A user also must log in to do this.

Here are some redux actions. Any possible improvements you can see here?:

postActions.js

import axios from 'axios';

const fetch = require('node-fetch');

export const fetchPosts = () => function (dispatch) {
  dispatch({ type: 'FETCH_POSTS' });

  return axios.get('/posts')
    .then((response) => {
      dispatch({ type: 'FETCH_POSTS_FUFILLED', payload: response.data });
    })
    .catch((err) => {
      dispatch({ type: 'FETCH_POSTS_REJECTED', payload: err });
    });
};

export const deletePosts = (posts, cb) => function (dispatch) {
  axios.delete('/delete', { data: posts })

    .then((response) => {
      dispatch({ type: 'DELETE_POSTS_FUFILLED', payload: response.data });
    });
};

export const newPost = (title, body, date) => function (dispatch) {
  return axios.post('/newpost', {
    title,
    body,
    author: 'testUser',
    date,

  })
    .then((response) => {
      dispatch({ type: 'NEW_POST', payload: response.data });
      console.log(response);
    });
};

export const findSinglePost = postId => function (dispatch) {
  dispatch({ type: 'FIND_SINGLE_POST', payload: postId });
};

export const sendEdit = editStatus => function (dispatch) {
  dispatch({ type: 'CHANGE_EDIT_STATUS', payload: editStatus });
};

export const handleSave = changes => function (dispatch) {
  axios.put('/edit', { data: changes })

    .then((response) => {
      dispatch({ type: 'EDITED_POST', payload: response.data });
    });
};

export const load = data => ({ type: 'LOAD', payload: data });

export const changeRedirect = bool => function (dispatch) {
  dispatch({ type: 'CHANGE_REDIRECT_STATUS', payload: bool });
};

export const checkBoxChange = postId => function (dispatch) {
  dispatch({ type: 'CURRENT_POST', payload: postId });
};

Please give me any feedback you can think of. I'm trying to get better at React/Angular.

You can turn the action types to constants.
Have action creators

https://redux.js.org/recipes/reducing-boilerplate/

In deletePosts you're passing a cb that's never used
For readability I would always wrap args in ( ) in arrow function (params)
A trivial comment/preference... I would make sure I use data in args so I don't have to assign it to prop axios.put('/edit', { data: changes }) I would just doaxios.put('/edit', { data })`


Functions that return a variable I'm simple looking for what the general preference is between the below options:

anon function directly to variable

middleware.customer = function(req, res, next){
    req.customer = function(){
        var customer = false;
        if(req.method == "POST") customer = req.body;
        if(req.method == "GET") customer = req.query;
        return customer;
    }();
    return next();
}

raw build variable up

middleware.customer = function(req, res, next){
    req.customer = false;
    if(req.method == "POST") req.customer = req.body;
    if(req.method == "GET") req.customer = req.query;
    return next();
}

complete separation

var customer = function(req){
    var customer = false;
    if(req.method == "POST") customer = req.body;
    if(req.method == "GET") customer = req.query;
    return customer;
}

middleware.customer = function(req, res, next){
    req.customer = customer(req);
    return next();
}

I think B is the best of the three, but here's another alternative using a simple lookup:

middleware.customer = function(req, res, next){
  var methods = {POST: req.body, GET: req.query};
  req.customer = methods[req.method] || false;
  return next();
}


Pulls href tags using BeautifulSoup with Python As if this isn't extremely obvious, I am a new coder.

My code works, but is quite far from ideal. I'm also not sure if there are unnecessary bits, since I've been trying to get this to work for a few days on and off, so I've started and stopped multiple times.

The goal is to have cleaner code, with a #comment on each line (unless extremely basic) so as to improve my annotation habits. Please let me know what you would do to improve it overall.

import urllib.request as ur
from bs4 import BeautifulSoup

url = str(input('Enter URL- ')) #convert input to string
html = ur.urlopen(url).read() #read html
soup = BeautifulSoup(html, "html.parser") #retrieve all of the anchor tags
Count_ = int(input('Enter count: ')) #convert input to integer
pos_1 = int(input('Enter position: ')) #convert input to integer
tags = soup('a')
final = '' #url of name list before break
curpos = ''
print('Retrieving: ', url) #prints starting point/url
count = int(Count_) + 1
while count > 1 : #starting a definite loop that goes until count is smaller than 1
    pos = 0
    for tag in tags :
        if pos == int(pos_1) - 1 : #conditional statement regarding position
            curpos = tag.get('href', None)
            break
        pos = pos + 1 #increases value of pos for each tag
    final = curpos #
    url = str(curpos) #
    html = ur.urlopen(url).read()
    soup = BeautifulSoup(html, "html.parser")
    tags = soup('a')
    count = count - 1 #for every iteration in the loop, subtract 1 from the value of count
    print('Retrieving: ', final)

General Observations

instead of manually looking for a tag a desired position and handling pos increment in the loop, I think you could just simply get the value by index:

curpos = tags[int(pos_1) - 1].get('href', None)

count = count - 1 could be simplified as count -= 1
follow the PEP8 lower_case_with_underscores variable naming guideline
what if you prefix the variable names containing user-defined values with input_ (see below)?
and, I think a "for" loop with a negative step would be a simpler solution that the while loop here:

for count in range(int(Count_) + 1, 1, -1):
    # ...

Or, to bring it one step further, what if we apply a generally easier to follow recursive flow instead of the iterative approach you currently have. The base condition for the recursion could be the input count reaching 1. And, we'll improve on DRY with that function as well.

Web-Scraping

you don't have to call .read() on the result of .urlopen() as BeautifulSoup also accepts file-like objects:

soup = BeautifulSoup(ur.urlopen(url), "html.parser")

switching from html.parser to lxml may help drastically improve HTML-parsing performance
instead of using urllib(), you could switch to requests and re-use a session which would help avoid an overhead of re-establishing network connection to the host on every request
you could use SoupStrainer to let BeautifulSoup parse only the a elements
you should also account for relative links and use urljoin() to combine base urls and relative links

The code with the above and other improvements applied:

from urllib.parse import urljoin

import requests
from bs4 import BeautifulSoup, SoupStrainer

only_links = SoupStrainer('a')

def follow_link(session, url, position, count):
    """Follows a link at a given "position" "count" number of times."""
    if count <= 1:
        return

    print('Retrieving: ', url)
    response = session.get(url)

    soup = BeautifulSoup(response.content, "lxml", parse_only=only_links)
    links = soup('a')
    next_url = links[position - 1].get('href', None)

    return follow_link(session, urljoin(url, next_url), position, count - 1)

if __name__ == '__main__':
    input_url = str(input('Enter URL- '))
    input_count = int(input('Enter count: '))
    input_position = int(input('Enter position: '))

    with requests.Session() as session:
        follow_link(session, input_url, input_position, input_count)

Afterthoughts

what if there is no link at the desired position available on some page?
if we once get a link which links itself, this code in this state would get stuck on this page alone until the count is exchausted

  The goal is to have cleaner code, with a #comment on each line (unless extremely basic) so as to improve my annotation habits.

Comments on each line could overall decrease readability of the code and they are, in essence, extra information and weight you need to make sure stay up to date with the actual code. Self-documented code is something you should strive to achieve, using comments as an extra measure used to explain the "why" some decisions were made and not the "how".


CharacterEscaper - further optimizable? Bugs? I have been working on a character escaper for a production server that receives quite a few requests per second (sometimes in the range of 400).

We forward some "query" requests to apache Lucent/SOLR, and I need to escape characters.

Containing also feedback from programmers-stackexchange , the code below is the result.

Can anyone see any bugs or additional ways to optimize?

String query = "http://This+*is||a&&test(whatever!!!!!!)";
char[] queryCharArray = new char[query.length()*2];
char c;
int length = query.length();
int currentIndex = 0;
for (int i = 0; i < length; i++)
{
    c = query.charAt(i);
    if(mustBeEscaped[c]){
      if('&'==c || '|'==c){
        if(i+1 < length && query.charAt(i+1) == c){
            queryCharArray[currentIndex++] = '\\';
            queryCharArray[currentIndex++] = c;
            queryCharArray[currentIndex++] = c;
            i++;
        }
      }
      else{
        queryCharArray[currentIndex++] = '\\';
        queryCharArray[currentIndex++] = c;
      }
    }
    else{
        queryCharArray[currentIndex++] = c;
    }
}

query = new String(queryCharArray,0,currentIndex);

System.out.println("TEST="+query);

private static final boolean[] mustBeEscaped = new boolean[65536];
static{
mustBeEscaped[':']=  //
  for(char c: "\\?+-!(){}[]^\"~*&|".toCharArray()){
     mustBeEscaped[c]=true;
  }
}

First I have to be honest and say that this code puts me into WTF-Mode very early on, but that must not be a bad thing. But what took me by surprise is the lack of comments, there are some edge-cases in there I can not make out why they're the way they're.

On the latter, changing, suggestions I did only perform minor profiling by simply running it in a loop with 1 million iterations and seeing if the time differs by much.

Rule of thumb: Never roll your own security!

Your indentation/brace-style is a mess. Java normally uses a modified K&R-Style, with the opening brace on the same line.

function test() {
    if (condition) {
        // Stuff
    } else {
        // Stuff
    }
}

You should fix that first.

Some of your names can be improved. F.e. queryCharArray might be better named escapedQuery.

You might want to extract all hard coded chars at least into static final variables for readability.

Your loop-beginning can be changed to:

char[] queryCharArray = new char[query.length() * 2];
int currentIndex = 0;
    for (int i = 0; i < query.length(); i++) {
        char c = query.charAt(i);

This improves readability by a lot.

Your loop can be optimized to this:

char c = query.charAt(i);
if (mustBeEscaped[c]) {
    queryCharArray[currentIndex++] = '\\';
    if ('&' == c || '|' == c) {
        if (i + 1 < query.length() && query.charAt(i + 1) == c) {
            queryCharArray[currentIndex++] = c;
            i++;
        }
    }
}
queryCharArray[currentIndex++] = c;

This removes unnecessary else-blocks.

if ('&' == c || '|' == c) {
    if (i + 1 < query.length() && query.charAt(i + 1) == c) {

You should leave a comment here explaining why these characters are special. Extracting them into another array might be a good idea.

if (skipNextOccurence[c]) {

Putting all this together (and renaming the constants to fit the Java naming conventions), I ended up with this in my tests:

private static final boolean[] MUST_BE_ESCAPED = new boolean[65536];
private static final boolean[] SKIP_NEXT_OCCURENCE = new boolean[65536];
private static final char ESCAPE_CHARACTER = '\\';

static {
    for (char c : "\\?+-!(){}[]^\"~*&|".toCharArray()) {
        MUST_BE_ESCAPED[c] = true;
    }
    for (char c : "&|".toCharArray()) {
        SKIP_NEXT_OCCURENCE[c] = true;
    }
}

private static String escape(String query) {
    char[] escapedQuery = new char[query.length() * 2];
    int currentIndex = 0;

    for (int idx = 0; idx < query.length(); idx++) {
        char c = query.charAt(idx);

        if (MUST_BE_ESCAPED[c]) {
            escapedQuery[currentIndex++] = ESCAPE_CHARACTER;

            if (SKIP_NEXT_OCCURENCE[c]) {
                // Check if the next char is the same, and if yes add it to
                // the escapedQuery and make sure that it is skipped.
                if (idx + 1 < query.length() && query.charAt(idx + 1) == c) {
                    escapedQuery[currentIndex++] = c;
                    idx++;
                }
            }
        }

        escapedQuery[currentIndex++] = c;
    }

    return new String(escapedQuery, 0, currentIndex);
}

To elaborate on the possibilities to use a StringBuilder, or a Set, or pretty much everything else I tried to come up with which looked more readable...damn that thing is fast! Even using a StringBuilder slowed it down considerably. So as long as you're aware that you are, a little bit at least, abusing memory with that 65k array (which will end up as 65k * 8Byte on most platforms) it's fine with me.

Also don't forget that you allocate twice the amount of whatever is passed into this function. So if somebody feels funny and passes a 1Mb query into that, it will at least consume 3Mb (1+1*2) on it's way. That is, if the JVM is not smarter then I think and does not allocate all that at once.

Just use more comments and JavaDoc.


'any' class implementation I have made an any class in C++, base loosely on boost::any, but written differently. I am checking to see if I have done it correctly and that there are no mistakes in it:

class any
{
public:
    any()
        : dt(new data<void *>(0))
        {
        }

    template<class T>
        any(const T &value)
        : dt(new data<T>(value))
        {
        }

    any(any &rhs)
        : dt(rhs.dt->duplicate())
        {
        }

    ~any()
        {
        delete dt;
        }

    template<class T>
        T cast() const
        {
        if (type() == typeid(T))
            {
            return (reinterpret_cast<data<T> *>(dt)->val);
            }
        throw std::exception("invalid cast type");
        }

    template<class T>
        operator T() const
        {
        return (cast<T>());
        }

    template<class T>
        bool is() const
        {
        return (type() == typeid(T));
        }

    any &operator=(any &rhs)
        {
        if (this != &rhs)
            {
            delete dt;
            dt = rhs.dt->duplicate();
            }
        return (*this);
        }

    template<class T>
        any &operator=(const T &value)
        {
        delete dt;
        dt = new data<T>(value);
        return (*this);
        }

    any &swap(any &rhs)
        {
        std::swap(dt, rhs.dt);
        return (*this);
        }

    template<class T>
        bool operator==(const T &value) const
        {
        return (type() == typeid(T) &&
            cast<T>() == value);
        }

    bool operator==(any &rhs) const
        {
        return (type() == rhs.type() &&
            dt->cmp(rhs.dt));
        }

    template<class T>
        bool operator!=(const T &value) const
        {
        return (!((*this) == value));
        }

    bool operator!=(any &rhs) const
        {
        return (!((*this) == rhs));
        }

    const std::type_info &type() const
        {
        return (dt->type());
        }

protected:
    struct dummy
        {
    public:
        virtual const std::type_info &type() const = 0;
        virtual bool cmp(dummy *rhs) const = 0;
        virtual dummy *duplicate() = 0;
        };

    template<class T>
        struct data
            : public dummy
        {
    public:
        data()
            : val()
            {
            }

        data(const T &value)
            : val(value)
            {
            }

        ~data()
            {
            }

        const std::type_info &type() const
            {
            return (typeid(T));
            }

        bool cmp(dummy *rhs) const
            {
            return (val == reinterpret_cast<data<T> *>(rhs)->val);
            }

        dummy *duplicate()
            {
            return (new data<T>(val));
            }

        T val;
        };

    dummy *dt;
    };

Your code is in pretty much good shape but there are still several issues apart from what mentioned by ChrisW:

There are many cases in input arguments and return types of functions where you are not particularly careful about const/non-const and value vs. reference.
This code won't work for built-in arrays, hence neither for C-style strings. One way is to decay the type of the input argument before storage; built-in arrays are decayed to pointers in this case (which means array elements are not really copied).
The default constructor shouldn't allocate anything; initialize the data pointer to nullptr and provide member functions empty() and clear() to control the state of having/not having data.
Your assignment operators are unnecessarily complex, inefficient (by self-tests) and not exception-safe (if new throws, the current object is already destroyed). The most elegant solution to all these issues is the copy-swap idiom, where all actual work is done by constructors alone.
You don't need typeid to test for type equality; a lightweight (but low-level) solution without RTTI is here.
Type identification should be kept as an internal detail; the minimal required functionality is type equality by is(); don't expose type(), rather keep it as private as possible.
Type checking is a good thing, but for performance you should also provide unchecked access.
Casting is from a base to a derived class, so need not (and should not) be done with reinterpret_cast; rather, dynamic_cast / static_cast for checked / unchecked access. dynamic_cast to a reference type will automatically throw an std::bad_cast if the object is not of the right type, so there is not need to manually check with is(). This does need RTTI but is more elegant.
Storing empty objects (like function objects) is currently inefficient, as it does need extra space on top of the virtual function table. This can be solved by the empty base optimization, which is done automatically by using std::tuple.
Implicit conversion operators are a possible source for ambiguities and confusion; you may keep them if you need their convenience, but use carefully (e.g. try to explicitly initialize an object of the right type).
Comparison operators are a clear overkill (if you have them, why not also have arithmetic operators, and so on?). If you still want them, define them as non-member functions, using public members is and cast to implement them.
There are no move semantics.
There is no specialized binary (non-member) function swap. Defaulting to std::swap is not as efficient as it involves three move operations; without move semantics, things are even worse as it involves three copy operations.

I took the liberty to re-factor your code to a great extent, and here is the result, resolving all issues above:

class some
{
    using id = size_t;

    template<typename T>
    struct type { static void id() { } };

    template<typename T>
    static id type_id() { return reinterpret_cast<id>(&type<T>::id); }

    template<typename T>
    using decay = typename std::decay<T>::type;

    template<typename T>
    using none = typename std::enable_if<!std::is_same<some, T>::value>::type;

    struct base
    {
        virtual ~base() { }
        virtual bool is(id) const = 0;
        virtual base *copy() const = 0;
    } *p = nullptr;

    template<typename T>
    struct data : base, std::tuple<T>
    {
        using std::tuple<T>::tuple;

        T       &get()      & { return std::get<0>(*this); }
        T const &get() const& { return std::get<0>(*this); }

        bool is(id i) const override { return i == type_id<T>(); }
        base *copy()  const override { return new data{get()}; }
    };

    template<typename T>
    T &stat() { return static_cast<data<T>&>(*p).get(); }

    template<typename T>
    T const &stat() const { return static_cast<data<T> const&>(*p).get(); }

    template<typename T>
    T &dyn() { return dynamic_cast<data<T>&>(*p).get(); }

    template<typename T>
    T const &dyn() const { return dynamic_cast<data<T> const&>(*p).get(); }

public:
     some() { }
    ~some() { delete p; }

    some(some &&s)      : p{s.p} { s.p = nullptr; }
    some(some const &s) : p{s.p->copy()} { }

    template<typename T, typename U = decay<T>, typename = none<U>>
    some(T &&x) : p{new data<U>{std::forward<T>(x)}} { }

    some &operator=(some s) { swap(*this, s); return *this; }

    friend void swap(some &s, some &r) { std::swap(s.p, r.p); }

    void clear() { delete p; p = nullptr; }

    bool empty() const { return p; }

    template<typename T>
    bool is() const { return p ? p->is(type_id<T>()) : false; }

    template<typename T> T      &&_()     && { return std::move(stat<T>()); }
    template<typename T> T       &_()      & { return stat<T>(); }
    template<typename T> T const &_() const& { return stat<T>(); }

    template<typename T> T      &&cast()     && { return std::move(dyn<T>()); }
    template<typename T> T       &cast()      & { return dyn<T>(); }
    template<typename T> T const &cast() const& { return dyn<T>(); }

    template<typename T> operator T     &&()     && { return std::move(_<T>()); }
    template<typename T> operator T      &()      & { return _<T>(); }
    template<typename T> operator T const&() const& { return _<T>(); }
};

I call it some instead of any because I prefer to hold something rather than anything :-) Plus, any is a common name for a function (like all), which is not the case for some.

Members cast() provide type-checked access, while members _() (shortest notation) provide unchecked access. I have chosen to give conversion operators unchecked access for performance but you are free to change that or remove them altogether.

I've also made a live example, including an extensive series of tests to demonstrate (almost) all possible uses.

Note that base (your dummy) now has a virtual destructor.

There is one more issue: using the free store is serious performance bottleneck when it comes to small types. What you can do is define a particular size to be allocated on stack, and only use the free store for larger objects, similar to short (or small) string optimization.

More generally, it is a good idea to parametrize the implementation with respect to how memory management (allocation, deallocation) is done by providing a kind of allocator object. any is really a container, even if of at most one object.

I have extended the implementation towards this direction but I will probably post this as a separate answer.


Even Fibonacci numbers under 4,000,000 (Project Euler #2) Prompt:

  Each new term in the Fibonacci sequence is generated by adding the
  previous two terms. By starting with 1 and 2, the first 10 terms will
  be:

    1, 2, 3, 5, 8, 13, 21, 34, 55, 89, ...

  By considering the terms in the Fibonacci sequence whose values do not
  exceed four million, find the sum of the even-valued terms.

I'd like advice on:

Using mathematical formulas instead of loops to calculate this
Making the code more efficient

This is my solution:

   function EvenFibonacciNumbers(){
            var value=parseInt(document.getElementById("input").value);
            var sum=0;
            var temp=0;
            var num2=1;
            var num1=1;

            while(num2<value){
                temp=num1+num2;
                num1=num2;
                num2=temp;
                if(num2%2==0){
                    sum+=temp;
                }
            }

            //document.getElementById("output").value=sum;
            alert("result ="+sum);
        }

Naming

var num1

That doesn't tell me what the variable is and does.  Is it the smallest Fibonacci number in the two needed to calculate the next?
Operators
Using spaces around your operators helps readability, which helps prevent bugs.
Scope
temp is only used in the while() loop, so you can reduce its scope.
Algorithm
The Fibonacci numbers go even/odd/odd/even/odd/odd... There is a good reason for this - they start with an even number (0), and an odd number (1). Even numbers plus odd numbers are odd numbers, so we get another odd number (1). Odd numbers plus odd numbers are even numbers, so we know this pattern will continue. With this knowledge, we only need to calculate every 3rd number.


Combine neighboring parts of an array based on a condition I have written this code which takes a one dimensional array, for example:

{"abc","ade","sss","fgw","asd","lka","o"}

and turns it into an array of arrays, combining the elements which do not meet the condition.

For the condition m=>!m.Contains("a") for example, the output will be:

{{"abc","ade"},{"sss"},{"fgw"},{"asd","lka"},{"o"}}

The code works, but i suspect there is a linq way to do it, and refrain from using temporary variables that i dislike.

    public static string[][] combineBy(this string[] inp,Func<string,bool> f)
    {
        List<string[]> holdElements = new List<string[]>();
        while (inp.Length!=0)
        {
            if (f.Invoke(inp[0]))
            {
                holdElements.Add(new string[] { inp[0] });
                inp = inp.Skip(1).ToArray();
            }
            else
            {
                var toAdd = inp.TakeWhile(n => !f.Invoke(n));
                holdElements.Add(toAdd.ToArray());
                inp = inp.Skip(toAdd.Count()).ToArray();
            }
        }
        return holdElements.ToArray();
    }

Some general remarks about your code:

combineBy use PascalCase for method names
f it's not a good parameter name, if it's a condition function they are usualy named predicate
predicate.Invoke(...) - you can call it directly with predicate(...)
values.TakeWhile(n => !predicate(n)) - do not enumerate it twice, call ToArray and use it in the subsequent lines
if you like it the LINQ way you can write values.Any() instead of values.Length != 0 and values.First() instead of values[0] also this new string[] { inp[0] } could be turned into values.Take(1).ToArray()
don't use abbreviations for variables like inp or f, use full names like values and predicate

x

public static string[][] CombineBy(this string[] values, Func<string, bool> predicate)
{
    var result = new List<string[]>();
    while (values.Any())
    {
        if (predicate(values.First()))
        {
            result.Add(values.Take(1).ToArray());
            values = values.Skip(1).ToArray();
        }
        else
        {
            var toAdd = values.TakeWhile(n => !predicate(n)).ToArray();
            result .Add(toAdd);
            values = values.Skip(toAdd.Length).ToArray();
        }
    }
    return result.ToArray();
}

Unfortuantely your code doesn't seem to work and returns invalid results. This should do:

public static List<List<string>> CombineBy(this string[] values, Func<string, bool> predicate)
{
    var result = new List<List<string>>();

    foreach (var value in values)
    {
        if (predicate(value))
        {
            if (!result.Any() || !predicate(result.Last().Last()))
            {
                result.Add(new List<string>());
            }
            result.Last().Add(value);
        }
        else
        {
            result.Add(new List<string> { value });
        }
    }

    return result;
}

or even without loops:

public static List<List<string>> CombineBy3(this string[] values, Func<string, bool> predicate)
{
    var result = values.Aggregate(new List<List<string>>(), (current, next) =>
    {
        if (predicate(next))
        {
            if (!current.Any() || !predicate(current.Last().Last()))
            {
                current.Add(new List<string>());
            }
            current.Last().Add(next);
        }
        else
        {
            current.Add(new List<string> { next });
        }
        return current;
    });
    return result;
}


Separate method for a single line code? Or embed in the caller function? I'm having an argument with my boss about whether or not it's better to have a separate function for a single line of code.

The code reads something like:

while (count <= poscount)
{
    key = etc.substr(pos2prev+1, pos-pos2prev-1);
    std::string::iterator end_pos = std::remove(key.begin(), key.end(), ' ');
    key.erase(end_pos, key.end());

    val = etc.substr(pos+1, pos2-pos-1);

    //process key and val
    DEBUG("{\n\t%s:%s\n}\n", key.c_str(), val.c_str());
    json_object_object_add(paramList, key.c_str(), json_object_new_string(val.c_str()));

    count++;
    pos2prev = pos2;
    pos = etc.find("=", pos+1);
    pos2 = (count == poscount) ? etc.find("\0",pos2+1) : etc.find("&", pos2+1);
}

Is it better coding style to include the line

json_object_object_add(paramList, key.c_str(), json_object_new_string(val.c_str()));

in a separate function name such as processKeyVal(), or leave it as-is?

I'm guessing that your boss's argument is that a well named function would make the code more readable, and I agree with that in general. I often do that myself even when the function contains a single line.

In this particular example I would not bother though. The comment above the json-call says the same thing as the function name would, and there are no obvious advantages to make a function for a single call.

So if your question is specifically for this example, then I agree with you - keep it as it is, but if you want a more general answer, then I agree with your boss.

For example - if this one-liner is something that you use in more than one place, i.e. if calling the "processKeyVal()" function in more than one place makes sense. Then I say you should make a function of it even if it would contain only one line of code.

Or to put it another way - it is not (always) the number of code lines that should determine if you make a separate function of a code snippet.


General insert function I am new to VB.NET and one of the functions I wrote is below.
The idea behind it is to just send a dictionary and table name to the function, get the result and then return.

Is this good practice or just silly?

Public Function dbInsert(ByRef EntryDetails As Dictionary(Of String, String),ByRef table as String) As Integer
    Dim command As new SqlCeCommand

    ' Loop over entries.
    Dim fieldNames() As String = {}
    Dim fieldsValues() As String = {}
    Dim pair As KeyValuePair(Of String, String)
    For Each pair In EntryDetails
        ReDim Preserve fieldNames(0 To UBound(fieldNames) + 1)
        fieldNames(UBound(fieldNames)) = pair.Key
        command.Parameters.AddWithValue("@" + pair.Key, pair.Value)
    Next

    command = New SqlCeCommand("INSERT INTO "+table+" (" + String.Join(",", fieldNames) + ")", connection.DbPublic)

    Dim newProdID As Int32 = 0
    Try
        command.ExecuteNonQuery()

        Dim cmd As New SqlCeCommand("SELECT @@IDENTITY", conn)
        newProdID = Convert.ToInt32(cmd.ExecuteScalar())
        'Console.WriteLine("added: {0}", newProdID)
        Return newProdID
    Catch e As Exception
        Console.WriteLine("Unable to insert: {0}", e.Message)
        Return 0
    End Try
End Function

This approach is not much good, but may be suit your required functionality. The method name is dbInsert and you
  are passing table and data in dictionary. you can make it much better by using better naming convention and parameter types.

On the place of this create a method with following parameter as:
GetIdentityOnInsert(string sqlQuery, CommandType commandType, SqlParameterCollection sqlParameters)

query will be: query = "Insert into tableName value(@param1,@param2);SELECT @@IDENTITY;"

Public Function GetIdentityOnInsert(ByVal sqlQuery as String, ByVal commandType as Commandtype, ByVal sqlParameters as SqlParameterCollection)
Try
Using connection As New SqlConnection(connectionString)

        ' Create the command and set its properties.
        Dim command As SqlCommand = New SqlCommand()
        command.Connection = connection
        command.CommandText = sqlQuery
        command.CommandType = commandType
        Dim parameter As New SqlParameter()

        For Each parameter In sqlParameters
            command.Parameters.Add(parameter);
        Next

        ' Open the connection and execute the reader.
        connection.Open()
        Dim newProdID As Int32 = 0
        ' try ExecuteScalar() if it gives error then replace it to ExecuteNonQuery()
        ' and replace the below code as in your question.
        newProdID = command.ExecuteScalar()

            'Dim cmd As New SqlCeCommand("SELECT @@IDENTITY", conn)
            'newProdID = Convert.ToInt32(cmd.ExecuteScalar())
            'Console.WriteLine("added: {0}", newProdID)
         Return newProdID

End Using
Catch e As Exception
    Console.WriteLine("Unable to insert: {0}", e.Message)
    Return 0
End Try
End Function

Hope this help you to create you Data Access Layer. you can extend ExecuteNonQuery with the above method.. just you have to customize little bit..


Ruby on Rails application controller My application controller keeps growing with small helper methods that I use in both my controller and views, is it okay? or what do you recommend doing?

class ApplicationController < ActionController::Base
  protect_from_forgery with: :exception

  helper_method :resource_name, :resource, :devise_mapping, :resource_class, :is_admin?, :is_representative?, :is_shipper?,
    :is_agent?, :is_operation_completed?, :is_fcl_exw?, :is_fcl_exw_info_stage_completed?, :is_fcl_exw_info_requested?, :is_fcl_exw_info_confirmed?,
    :is_pricing_representative?, :is_fcl_exw_quotation_confirmed?

  # Roles detection helpers
  def is_admin?
    current_role == 'admin' ? true : false
  end

  def is_representative?
    current_role == 'representative' ? true : false
  end

  def is_shipper?
    current_role == 'shipper' ? true : false
  end

  def is_agent?
    current_role == 'agent' ? true : false
  end

  def is_pricing_representative?
    current_role == 'pricing_representative' ? true : false
  end

  # Devise helpers
  def resource_name
    :user
  end

  def resource
    @resource ||= User.new
  end

  def resource_class
    User
  end

  def devise_mapping
    @devise_mapping ||= Devise.mappings[:user]
  end

  # Operation helpers
  def is_operation_completed?(operation_id)
    GeneralCargoInfo.find_by(operation_id: operation_id).nil? ? false : true
  end

  # FCL-EXW helpers
  def is_fcl_exw_info_stage_completed?(operation_id)
    FclExwCargoInfo.find_by(operation_id: operation_id).nil? ? false : true
  end

  def is_fcl_exw?(operation_id)
    Operation.find(operation_id).modality == 'FCL - EXW' ? true : false
  end

  def is_fcl_exw_info_requested?(operation_id)
    Operation.find(operation_id).fcl_exw_info_requested
  end

  def is_fcl_exw_info_confirmed?(operation_id)
    Operation.find(operation_id).fcl_exw_info_confirmed
  end

  def is_fcl_exw_quotation_confirmed?(operation_id)
    Operation.find(operation_id).fcl_exw_quotation_confirmed
  end

  private
    def current_role
        current_role = current_user.nil? ? 'Not logged in' : Role.find(current_user.role_id).name
    end

    def require_new_operation_permission
      check_permission('representative', 'agent')
    end

    def check_permission(*roles)
      unless roles.include? current_role
        flash[:alert] = "Access denied"
        redirect_back(fallback_location: authenticated_root_path)
      end
    end

end

You can move helper methods to ApplicationHelper and simply include it in Application controller, so your code will look like:

ApplicationController:

class ApplicationController < ActionController::Base
  include ApplicationHelper
  protect_from_forgery with: :exception

  private
    def current_role
        current_role = current_user.nil? ? 'Not logged in' : Role.find(current_user.role_id).name
    end

    def require_new_operation_permission
      check_permission('representative', 'agent')
    end

    def check_permission(*roles)
      unless roles.include? current_role
        flash[:alert] = "Access denied"
        redirect_back(fallback_location: authenticated_root_path)
      end
    end
end

ApplicationHelper:

module ApplicationHelper
  # Roles detection helpers
  def is_admin?
    current_role == 'admin' ? true : false
  end

  def is_representative?
    current_role == 'representative' ? true : false
  end

  def is_shipper?
    current_role == 'shipper' ? true : false
  end

  def is_agent?
    current_role == 'agent' ? true : false
  end

  def is_pricing_representative?
    current_role == 'pricing_representative' ? true : false
  end

  # Devise helpers
  def resource_name
    :user
  end

  def resource
    @resource ||= User.new
  end

  def resource_class
    User
  end

  def devise_mapping
    @devise_mapping ||= Devise.mappings[:user]
  end

  # Operation helpers
  def is_operation_completed?(operation_id)
    GeneralCargoInfo.find_by(operation_id: operation_id).nil? ? false : true
  end

  # FCL-EXW helpers
  def is_fcl_exw_info_stage_completed?(operation_id)
    FclExwCargoInfo.find_by(operation_id: operation_id).nil? ? false : true
  end

  def is_fcl_exw?(operation_id)
    Operation.find(operation_id).modality == 'FCL - EXW' ? true : false
  end

  def is_fcl_exw_info_requested?(operation_id)
    Operation.find(operation_id).fcl_exw_info_requested
  end

  def is_fcl_exw_info_confirmed?(operation_id)
    Operation.find(operation_id).fcl_exw_info_confirmed
  end

  def is_fcl_exw_quotation_confirmed?(operation_id)
    Operation.find(operation_id).fcl_exw_quotation_confirmed
  end
end


Criticism on x86_64 nasm assembly strToInt and printInt implementation I am pretty new to assembly I would like criticism on two "methods" I have written. One converts string to int and the other prints an int to stdout. Any advise helps out.
bits 64

section .data
endl db 10, 0
endlLength equ $ - endl

section .bss
   string resb 100

section .text
global _start
global _strToInt
global _printInt

_start:
   mov rbp, rsp

   ;get int from user
   mov rax, 0
   mov rdi, 0
   lea rsi, [string]
   mov rdx, 100
   syscall

   ;make string to int
   push string
   call _strToInt
   pop r8

   push rax
   call _printInt
   pop  rax

   mov rax, 1
   mov rdi, 1
   mov rsi, endl
   mov rdx, endlLength
   syscall

   mov rax, 60
   mov rbx, 0
   syscall

   ;last value pushed to stack must be address to string. stores int in rax
_strToInt:
  ;prolog
  push rbp
  mov rbp, rsp
  ;save registers
  push rbx

  ;actual code
  xor rax, rax
  mov rbx, 10          ;will be used to multiply by 10 later

  mov rcx, [rbp + 16]
  cmp byte[rcx], '-'   ;check to see if value is negetive
  jne _convertStringLoop
  inc rcx              ;increment the address

  _convertStringLoop:
     xor rdx, rdx      ;clear rdx
     mul rbx           ;multiply rax by 10
     mov dl, [rcx]     ;get 1 byte from rcx address
     sub dl, '0'       ;seb by '0' to get actual value
     add rax, rdx      ;add it to rax
     inc rcx
  cmp byte[rcx], 10    ;see if new line char. exit if new line char
  je _exitConvertStringLoop
  cmp byte[rcx], 0     ;see if end of line char. exit if end of line
  jne _convertStringLoop

  _exitConvertStringLoop:
  mov rcx, [rbp + 16]
  cmp byte[rcx], '-'   ;if not negetive jump
  jne _exitStrToInt
  not rax              ;1's complement (make negetive)
  inc rax              ;2's complement (make negetive)

  _exitStrToInt:
  ;restore registers
  pop rbx
  ;epilog
  pop rbp
  ret

;last value pushed to stack will be printed
_printInt:
  ;prolog
  push rbp
  mov rbp, rsp
  ;save registers
  push rbx

  ;actual code
  mov rsi, rsp
  mov rax, [rbp + 16] ;get the value that user wants to print
  mov rbx, 10         ;will be used to divide by 10 later
  xor rcx, rcx

  cqo
  cmp rdx, -1         ;check to see if negetive
  jne _divisionLoop   ;if not negetive jump

  ;print negetive sign
  mov [rsi + 1], byte '-'
  mov rax, 1
  mov rdi, 1
  inc rsi
  mov rdx, 1
  syscall
  dec rsi

  ;convert to positive number
  mov rax, [rbp + 16]
  ;imul rax, -1
  dec rax
  not rax
  xor rcx, rcx

  _divisionLoop:
     xor rdx, rdx
     div rbx        ;divides number by 10 to move over last digit into rdx reg
     add dl, '0'
     dec rsi
     mov [rsi], dl
     inc rcx        ; count for how many digits added to stack
  cmp rax, 0
  jnz _divisionLoop ;jump if the division did not result in a zero

  ;print all the values
  mov rax, 1
  mov rdi, 1
  mov rdx, rcx
  syscall

  ;restore register
  pop rbx
  ;epilog
  pop rbp
  ret

One more thing I wanted to ask was is it better to do
     mov rax, -20
     mov rbx, -1
     imul rbx

or
     mov rax, -20
     dec rax
     not rax

when I know a number is negetive and I want to turn it to a positive.

Warming up
A good thing that I see missing from your program is writing comments about the function numbers and more.
mov rdi, 0    ; STDIN
mov rax, 0    ; SYS_READ
syscall
...
mov rdi, 1    ; STDOUT
mov rax, 1    ; SYS_WRITE
syscall

mov rdi, 0    ; OK
mov rax, 60   ; SYS_EXIT
syscall

Here's an improvement that you can make:

section .data
endl db 10, 0
endlLength equ $ - endl

The output to STDOUT is length driven. It's not useful to zero terminated this newline. All you could get is that ASCII character 0 gets displayed needlessly.
There are some errors in your code

mov rax, 60
mov rbx, 0
syscall

The first argument must go in RDI. You wrote RBX. Habit from 32-bit ?

_convertStringLoop:
  xor rdx, rdx      ;clear rdx
  mul rbx           ;multiply rax by 10
  mov dl, [rcx]     ;get 1 byte from rcx address
  sub dl, '0'       ;seb by '0' to get actual value
  add rax, rdx      ;add it to rax

You have misplaced that xor rdx, rdx instruction. The mul rbx instruction that follows will modify RDX as part of its operation. If the input that you get from the user at the keyboard is really big, then RDX can become non-zero! But wait! You need RDX to be zero so the addition can work correctly...
This will work fine zero-extending DL into EDX into RDX:
_convertStringLoop:
   mul   rbx              ; multiply rax by 10
   movzx edx, byte [rcx]  ; Get 1 byte from RCX address and put in RDX
   sub   dl, '0'          ; seb by '0' to get actual value
   add   rax, rdx         ; add it to rax

and this will work too and allows to not use RBX at all (saves many instructions):
_convertStringLoop:
   imul  rax, 10          ; multiply rax by 10
   movzx edx, byte [rcx]  ; Get 1 byte from RCX address and put in RDX
   sub   dl, '0'          ; seb by '0' to get actual value
   add   rax, rdx         ; add it to rax

;print negetive sign
mov [rsi + 1], byte '-'
mov rax, 1
mov rdi, 1
inc rsi
mov rdx, 1
syscall
dec rsi

This code overwrites the value of RBX that you're trying to preserve on the stack!
You need to offset by a negative number or else decrement RSI beforehand:
; print negative sign
dec  rsi
mov  byte [rsi], '-'
mov  rdx, 1
mov  rdi, 1   ; STDOUT
mov  rax, 1   ; SYS_WRITE
syscall
inc  rsi

Some better ways of doing things

cqo
cmp rdx, -1         ;check to see if negetive
jne _divisionLoop   ;if not negetive jump

You can test if RAX contains a negative number simply by testing the register with itself and then inspecting the sign flag:
test rax, rax
jns  _divisionLoop   ; RAX is positive

cmp byte[rcx], '-'   ;if not negetive jump
jne _exitStrToInt
not rax              ;1's complement (make negetive)
inc rax              ;2's complement (make negetive)

The instruction set offers you the NEG instruction to negate a number:
cmp  byte [rcx], '-'   ; If not negative jump
jne  _exitStrToInt
neg  rax

cmp rax, 0
jnz _divisionLoop ;jump if the division did not result in a zero

To find out if a register is 0, you can test it with itself and inspect the zero flag. This shaves off a byte and generally produces faster code:
test  rax, rax
jnz   _divisionLoop


Input memory buffer exceeding situation handling Please review the following code:

static int md5_encode(const char *  input_md5  , char *  output  , int size)
{
  // checking pre-requisits //
  if( output == NULL || input_md5 == NULL || size == 0)
  {
   printf( "main.cpp:md5_encode() error check for the prerequisites. \n");
   exit(0);
  }

  int input_length = strlen(input_md5);

  // null terminate the output first //
  output[0] = 0;
  char * buffer [10];

  for (int i=0; i<input_length,i++)
  {
    char current_char = input_md5 [i];
    // encode it //
    strcat(output,"uc");
    sprintf(buffer,"%d",current_char);
    strcat(output,buffer);

  }
  // check for the buffer overflow condtions //
  // if something goes out of the buffer then assert //
  if(size < strlen(output) )
  {
    printf("Warning: Possible buffer-overflow\n");
    return 0;
  }

  // return on success //
  return 1;
}

As you can see, what this does is to encode an md5 string into ucuc format. The output buffer is allocated by the caller and its size and address are passed as parameters.

The function just writes to the output buffer with 'strcat()' without any checking. Then afterwards it checks whether 'size' is less than the amount that was written. If it exceeds the limit, then the function gives an error message.

Any more suggestions than just a warning message? How could I improve this?

I'm not familiar with the "ucuc format", or what such an output format could be useful for.  A Google search for md5 ucuc format shows this question itself as the only relevant hit.

What are the expected inputs?  A 32-character string of hexadecimal digits?  If so, the longest possible output should be 32 repetitions of uc102, corresponding to an input of ffffffffffffffff.  (If the input can contain arbitrary characters, then allow for 6 bytes of output per byte of input, since there might be a negative sign.)  Why not just document the requirement for the caller to pass you an output buffer capable of storing 161 bytes?  Then do away with the size parameter.  You could return the number of bytes of output (excluding the \0 terminator).

Printing the error messages is not advised; it contaminates the output, and side effects limit the ability to reuse your code.  If you must print diagnostics, print to stderr instead.  You can use the __FILE__ and __FUNCTION__ macros for the error message.

A more common way to handle null inputs is to either let it crash, or use assertions.  An assertion failure message typically already includes the line number.  If you use assertions, use three separate assertions, not a combined one.  Calling exit() is unusual; calling exit(0) to exit successfully is even weirder.

Checking for possible buffer overflow at the end of the function is useless.  The damage would already be done, and your program could very well have crashed before reaching the check.

Your use of strlen() and strcat() is inefficient, especially when done in a loop.  Each of those calls is an O(n) operation, as it scans the buffer for the \0 terminator.  You already know exactly where those terminators are, since you composed the output string yourself.

/**
 * Encodes the input_md5 in ucuc format, whatever that means.
 * The caller must provide an output capable of holding 5 bytes
 * per byte of input, plus 1 byte for a null terminator.
 *
 * Returns the length of the output in bytes, excluding the null
 * terminator.
 */
static int md5_encode(const char *input_md5, char *output)
{
    // Check preconditions
    assert(input_md5);
    assert(output);

    int out_offset = 0;
    for (const char *in = input_md5; *in; in++)
    {
        // Encode it. sprintf() already adds a \0 terminator.
        out_offset += sprintf(output + out_offset, "uc%d", *in);
    }

    return out_offset;
}

In my opinion, two spaces of indentation per level is too stingy for readability, and also encourages inappropriately deep nesting.


Efficiently determine whether two arbitrarily-sized integers share a digit My current algorithm creates two new integers initialized to 0 (one for each input), then sets the nth bit to 1 if the integer's corresponding input contains the digit n. The bitwise AND of these integers is then 0 if the inputs don't share a digit.

I'm pretty sure there's a way to make this function more efficient. Is there something I'm missing? Example implementation (in Java) below.

boolean shareDigits(long a, long b)
{
    int aBits=0, bBits=0;
    if(a==0) aBits=1;
    if(b==0) bBits=1;
    while(a!=0)
    {
        aBits |= (1 << (a % 10));
        a/=10;
    }
    while(b!=0)
    {
        bBits |= (1 << (b % 10));
        b/=10;
    }
    return (aBits & bBits) > 0;
}

Edge cases

You don't handle negative integers very well. Specifically, the modulo operator, when performed on a negative integer with a positive divisor, may return a negative value. And when this negative value is something like, say, -2, then you get 1 << -2, which results in setting a high bit instead of the bits for 0-9.

Duplication

You currently have duplicated the code for a and b. By making use of smaller methods, you can get rid of the duplication:

int getBitsFor(long num)
{
    int bits=0;
    if(num==0) bits=1;
    while(num!=0)
    {
        bits |= (1 << (num % 10));
        num/=10;
    }
    return bits;
}

And then use like so:

boolean shareDigits(long a, long b)
{
    return (getBitsFor(a) & getBitsFor(b)) > 0;
}

Readability

It would help if you were to put spaces between operators and the operands. Basically, I find int aBits = 0 more readable than int aBits=0. The less effort I have to spend on interpreting the code, the more effort I can spend actually writing or improving the code.

For reference:

int getBitsFor(long num)
{
    int bits = 0;
    if(num == 0) bits = 1;
    while(num != 0)
    {
        bits |= (1 << (num % 10));
        num /= 10;
    }
    return bits;
}

Compare with the snippet above, see which feels easier to read for you.

Simplification

200_success pointed this out, but they deleted their answer (although I found that it made a good point):

If you wrote do-while loops instead, then you would not need the special case for num == 0:

int getBitsFor(long num)
{
    int bits = 0;
    do
    {
        bits |= (1 << (num % 10));
        num /= 10;
    } while (num != 0)
    return bits;
}

Performance

You want to know if there's a faster way check if integers share a digit.

To be honest, I don't know. Your implementation looks pretty fast. But your implementation also takes a constant amount of calculations related to the size of the input and makes no use of shortcuts.

One shortcut that I can come up with - if a number contains all the digits, then you don't need to continue.

But perhaps checking for this case makes the worst case slower again...

I also checked https://graphics.stanford.edu/~seander/bithacks.html which is a collection of "Bit Twiddling Hacks", but there doesn't seem to be an algorithm for checking for duplicated digits in there.


Processing student scores from the stdin As part of learning more C, I wrote this extremely trivial C program that takes Student scores from stdin and, upon termination, prints out the minimum score/maximum score and the average score. Can somebody review my code and tell me how I might be able to improve my code?

#include <stdio.h>

int main()
{
        int currentEntry;
        int startValue = 0;
        int counter = 0;
        int currentMax;
        int currentMin;
        double average;

        printf("Enter scores one-by-one, exit by entering -1\n");
        while(currentEntry != -1){
                if((scanf("%d",&currentEntry) == 1) && (currentEntry != -1)){
                        printf("Current score is %d\n",currentEntry);
                        counter++;
                        startValue += currentEntry;
                        average = startValue/counter;
                        if(counter == 1){
                                currentMax = currentEntry;
                                currentMin = currentEntry;
                        }
                        if(currentEntry > currentMax){
                                currentMax = currentEntry;
                        }
                        if(currentEntry < currentMin){
                                currentMin = currentEntry;
                        }
                }
        }
        printf("The average is %lf\n",average);
        printf("The maximum score is %d\n",currentMax);
        printf("The minimum score is %d\n",currentMin);
        return 0;
}

You should get into the habit of initialising variables whenever possible. It's a little more difficult here, since currentMax, currentMin, and currentEntry may seem not to have a sensible default value. However, all arithmetic values have an upper and a lower bound (these can be found in limits.h). It would make sense to initialise currentMax and currentMin based on these:

int currentMax = INT_MIN;
int currentMin = INT_MAX;

Note that if the user inputs -1 straight away, currentMax and currentMin will now print out predictable values. If you leave them uninitialised, they will print out whatever bits happen to be on the stack at that point in time (effectively, they will print out a random value).

Also, note that because these are now initialised with the min/max integer values respectively, you can remove the check for counter being 1.

I'd also initialise currentEntry and average, 0 and 0.0 seem like relatively reasonable defaults.

The name startValue is accurate at the beginning, but by the end, doesn't really reflect the starting value as you've been adding to it each time. Hence, I'd rather call it something like currentTotal.

Integer division in C can trip beginners up, the line:

average = startValue/counter;

isn't doing quite what you want it to be. Since startValue and counter are both integers, this will do an integer division, and then convert that to a double. Hence this will actually be the floor of the average. To fix this, you need to cast the numerator or denominator to a double:

average = (double)startValue / counter;

In the end, the code is pretty similar, but looks something like:

#include <stdio.h>
#include <limits.h>

int main()
{
    int currentEntry = 0;
    int currentTotal = 0;
    int counter = 0;
    int currentMax = INT_MIN;
    int currentMin = INT_MAX;
    double average = 0.0;

    printf("Enter scores one-by-one, exit by entering -1\n");

    while(currentEntry != -1) {
        if((scanf("%d", &currentEntry) == 1) && (currentEntry != -1)) {
            printf("Current score is %d\n", currentEntry);
            counter++;
            currentTotal += currentEntry;
            average = (double)currentTotal/counter;

            if(currentEntry > currentMax) {
                currentMax = currentEntry;
            }

            if(currentEntry < currentMin) {
                currentMin = currentEntry;
            }
        }
    }

    printf("The average is %lf\n",average);
    printf("The maximum score is %d\n", currentMax);
    printf("The minimum score is %d\n", currentMin);
    return 0;
}


Can I make my render method more efficient? I get great FPS with this as is, but I'm a performance freak and I don't know AffineTransform or the Java graphics libraries very well. I'm sure there is something I could be doing differently to make this faster, but I can't think of anything, other than it looks like the scale transformation gives me a big performance hit, probably because of the algorithm Graphics2D uses to draw the scaled pixels. I'd be fine with it using a different algorithm that doesn't blend colors if that would make things run faster, but I don't know where to even begin to try to do that.

I dumbed down my code to give you a good idea of what's going on (I hope). I'm only concerned about the render method.

public BufferedImage image = new BufferedImage(getWidth(), getHeight(), BufferedImage.TYPE_INT_RGB);
public Graphics2D graphics = image.createGraphics();

public void render(BufferedImage img, Vector2 center, Vector2 offset, double rotation, Vector2 scale) {
    graphics.setTransform((AffineTransform) defaultTransform.clone());
    graphics.rotate(rotation, center.x, center.y);
    graphics.scale(scale.x, scale.y);
    graphics.translate(center.x / scale.x + offset.x, center.y / scale.y + offset.y);
    graphics.drawImage(img, null, 0, 0);
}

public class Vector2 {
    double x = 0.0, y = 0.0;
}

Sometimes, the correct answer really is "Nope; it's all good."  It makes for a boring answer, but there it is.  It doesn't look like you can make this much faster, based on what you've provided.

One thing you might consider is checking the variables to see if the image manipulations are actually needed before calling the respective methods.  So, for example, something like...

graphics.setTransform((AffineTransform) defaultTransform.clone());
if(rotation != 0.0) {
    graphics.rotate(rotation, center.x, center.y);
}
if(scale.x != img.getWidth() || scale.y != img.getHeight()) {
    graphics.scale(scale.x, scale.y);
}
//...

This might give you a bit of performance where you don't need to bother going down into the rotate() and scale() methods.

Also, is there any way you can get rid of that (AffineTransform) cast?  Probably not, but that would speed you up ever so slightly.


Perl script to rename multiple files with vi(m) I'm new to Perl, learning on my own. I've wrote many scripts (utilities) for myself but never posted them online. I would really appreciate any feedback on this script before I post it on a forum.

The script takes list of files names on command line, writes the name list in tmp file and opens it with vi(m). After user is done renaming, it reads new names from tmp file and renames files accordingly. Script can also process symlinks, where it lists and renames the target file name and also updates the symlink to point to renamed file afterwards.

#!/usr/bin/perl -w
use strict;
use Getopt::Std;
use File::Temp qw( tempfile );
use File::Basename qw( basename dirname );

# HANDLING CMD ARGS
$Getopt::Std::STANDARD_HELP_VERSION = 1;
my $usage = \&main::HELP_MESSAGE;
my %opts;

sub main::VERSION_MESSAGE {
    print "virename v0.1\n";
}

sub main::HELP_MESSAGE {
    print << "EOF";
Usage:\n virename [OPTIONS] [file_names]

OPTIONS
 -s                act on symlink target (also update symlink)
 -v                be verbose
 --help            pirnt help and exit
 --version         version
EOF
}

if (@ARGV) {
    getopts( 'vs', \%opts ) or $usage->();
}
else {
    print "Not enough arguments!\n\n";
    $usage->();
    exit 1;
}

my ( @names, @new_names, %symlinks );

if ( $opts{s} ) {
    for my $name (@ARGV) {
        if ( my $target = readlink $name ) {
            my $target_name = basename $target;
            $symlinks{$target_name} = { 'symlink', $name, 'target', $target };
            push @names, $target_name;
        }
        else {
            push @names, $name;
        }
    }
}
else {
    @names = @ARGV;
}

die "Nothing to rename!\n" unless @names;

my ( $tmp, $tmp_name ) = tempfile();    # Tmp file

print {$tmp} join "\n", @names;         # Writing names to tmp file
if ( !system "vim $tmp_name 2>/dev/null" ) {
}
elsif ( !system 'vi', $tmp_name ) {
}
else { exit; }

seek $tmp, 0, 0;
chomp( @new_names = <$tmp> );           # Reading new names from tmp file

unlink $tmp_name;                       # Removing tmp file

exit if @names != @new_names;

#Renaming
for ( 0 .. $#names ) {
    my ( $new_name, $old_name );
    my $is_symlink = exists $symlinks{ $names[$_] };
    if ( $opts{s} && $is_symlink ) {
        $old_name = $symlinks{ $names[$_] }->{target};
        $new_name = dirname($old_name) . q{/} . $new_names[$_];
    }
    else {
        $old_name = $names[$_];
        $new_name = $new_names[$_];
    }

    if ( -e $new_name ) {    #Skip if file already exists
        print "'$new_name' already exits!\n" if $opts{v};
    }
    elsif ( rename $old_name => $new_name ) {
        if ( $opts{s} && $is_symlink ) {
            my $symlink = $symlinks{ $names[$_] }->{symlink};
            unlink $symlink;    # Removing old symlink
            symlink $new_name => $symlink    # Creating new symlink
              or warn "symlink: $!\n";
            print "'$symlink': '$old_name' => '$new_name'" if $opts{v};
        }
        else {
            print "'$old_name' => '$new_name'\n" if $opts{v};
        }
    }
    else {
        warn "failed to rename $old_name to $new_name: $!\n";
    }
}
__END__

That is a nice and useful script, although a few things could be improved.

You specified the -w command line option. You should instead use warnings. While the result is identical for such a small script, the warnings pragma allows for fine-grained control of warnings.
You do not have to specify main:: when defining subroutines. When no package is specified, then you are already in the package main. Reduce the syntactic clutter, and simply write sub VERSION_MESSAGE { ... }.
In HELP_MESSAGE, I'd use the heredoc-marker differently: I'd prefer it to be <<'EOF'. Rationale: Double quotes allow interpolation of variables. For a reader of the code, this increases the cognitive load: where do you interpolate something? Nowhere. If I specify the marker with single quotes, I can just skip to the end marker; knowing that nothing interesting happens in between.

I'd also rather not put a space between the << and the 'END'. The heredoc-marker is syntactically one single token, and you can't specify the marker with other quoting operators like << q(END).

As we now use single quotes, no escapes are available, so your \n won't work any more. No problem, just use a literal newline:

<<'EOF';
Usage:
 virename [OPTIONS] [file_names]
...
EOF

for the same effect.
Why do you put the HELP_MESSAGE sub into the scalar $usage? You can invoke the sub directly like HELP_MESSAGE() instead of the much more complicated $usage->().
sub VERSION_MESSAGE { print "virename v0.1\n"; }. Nope. The documentation for Getopt::Std may not make this perfectly clear, but your VERSION_MESSAGE sub receives some paramaters. The first of these is the filehandle you are expected to print to. Error messages or information that is not the primary output of your script should generally not go to STDOUT, but to STDERR. So we will rewrite that to

# callback for Getopt::Std
sub VERSION_MESSAGE {
  my ($handle) = @_;
  print {$handle} "virename v0.1\n";
}

Notice also the comment that tells a reader why you put this seemingly unused sub here and why you chose a name in offending uppercase.

The same considerations hold for HELP_MESSAGE, which should be rewritten as well.
print "Not enough arguments!\n\n"; ...; exit 1. Nope.

First, what you are expressing is very close to die, so we'll rather use that. The above rant about how error messages shouldn't go to STDOUT applies here as well, but using die fixes that. But we would like to print out the usage without having to call a sub that prints it out. The solution is to put this message into a variable, e.g.

my $help_message = <<'EOF';
...
EOF

# callback for Getopt::Std
sub HELP_MESSAGE {
  my ($handle) = @_;
  print {$handle} $help_message;
}

Now we can say

@ARGV or die "Not enough arguments!\n\n", $help_message;
getopts 'vs', \%opts or die $help_message;

When an unknown flag is among the arguments and getopts returns a false value, I not only print the usage (as you do), but also terminate execution. I think this is preferable than continuing with possibly wrong arguments.
A comment here and there would not hurt. E.g.

if ( $opts{s} ) {
  # If a file is a symlink, we need to find the actual target
  # and put that into @names instead.
  ...
}

A short paragraph telling a reader what you are about to do can really help.
What the hell?

if ( !system "vim $tmp_name 2>/dev/null" ) {
}
elsif ( !system 'vi', $tmp_name ) {
}
else { exit; }

I do understand what you are trying to express, but it isn't exactly obvious. There is a nicer way to do that. Also, don't fail with a normal exit code when there clearly was an error, and don't exit abnormally without an error message!

system("vim $tmp_name 2>/dev/null") == 0
  or system("vi", $tmp_name) == 0
  or die "Can't launch vim or vi\n";

Note: Don't write code that isn't self-explanatory without a comment. Also, If you're using empty bodies for an if in Perl, this should raise an eyebrow.
chomp( @new_names = <$tmp> ); # Reading new names from tmp file

Thankyou for the comment, but that code is fairly self-explaining. Ergo, this comment is unneccessary.

As this is the first occasion where the @new_names variable is used, it should also be declared here, and not further upwards. This would work as well:

chomp( my @new_names = <$tmp> );

… altough others may not like embedding declarations inside an argument list. Then:

my @new_names = <$tmp>;
chomp @new_names;

exit if @names != @new_names;. I covered this already: Don't exit abnormally without reflecting this in the error code, and don't abort execution without an error message telling the user what went wrong:

@names == @new_names or die "Number of filenames changed. Did you delete a line?\n";

In the loop, you could consider changing the definition of $is_symlink to

my $is_symlink = $opts{s} && exists $symlinks{ $names[$_] };

This reduced a tiny bit of code duplicatation.
The following if/elsif/else confuses two unrelated topics: Loop control and error handling.

if ( -e $new_name ) {    #Skip if file already exists
    print "'$new_name' already exits!\n" if $opts{v};
} elsif ( rename $old_name => $new_name ) {
    ...
} else {
    warn ...;
}

If you want to move on to the next iteration of the loop, just use next:

if ( -e $new_name ) {
  print ... if $opst{$v};
  next;
}

I am not sure if what you are printing is considered regular output, or an error message. I assume it is normal output, so my STDERR ranting does not apply here.

Your rename and the correspondig error handling (in form of a warn) are seperated by a dozen lines. I would move them closer together:

rename $old_name => $new_name or do {
  warn ...;
  next;
};
...;

The __END__ marker is generally useless, unless you keep some non-code resources in the same file, or when you pipe the code to the perl interpreter. All this marker does is to stop the parsing of the source file.


Funds transfer authorization I have the working code but it is pretty dirty. The goal is to input a number  in an application. If it satisfies the min/max logic then jump to the authorization part. Afterwards we can confirm it or reenter it or cancel it. If fails then re-input it. So totally we can only input twice.

 public virtual CallerAuthorization GetCallerAuthorizationResult(decimal debitBalance)
    {
        bool success;
        try
        {
            _debitBalanceAmount = debitBalance;
            GetFundAmountToTransfer();
            success = BuildAmountMinMaxLogicLoop();
        }
        catch (HangupException)
        {
            throw new CallHangupException();
        }
        if (!success)
        {
            success = ReenterAmount();
        }
        if (!success)
            return null;
        AuthorizeTransfer();
        var result = GetProceedInstruction();
        if (result == "1")
            return _callerAuthorization;
        if (result == "2")
        {
            success = ReenterAmount();
            if (!success)
            {
                CancelAuthorization();
                return null;
            }
            AuthorizeTransferReenterAmount();
            return _callerAuthorization;
        }
        if (result == "3")
        {
            CancelAuthorization();
            return null;
        }
        return null;
    }

The other methods:

 public virtual bool BuildAmountMinMaxLogicLoop()
    {
        var isContainingSpecialChars = CheckWhetherInputContainingSpecialChars();
        var isNotAuthorizable = IsNotAuthorizable();
        if (isNotAuthorizable)
        {
            if (isContainingSpecialChars)
            {
                PlayTransferAmountLogicError();
                return false;
            }
            PlayTransferAmountLogicError();
            return false;
        }
        return true;
    }

Also:

public virtual bool ReenterAmount()
    {
        PlayFundAmountToTransferPrompt();
        GetFundAmountToTransfer();
        var isNotAuthorizable = IsNotAuthorizable();
        if (isNotAuthorizable)
        {
            PlayGoodByeMessagePrompt();
            return false;
        }
        return true;
    }

    private void AuthorizeTransferReenterAmount()
    {
        GetAuthorizationResult();
        if (ResultInfo.Code == 0 && AuthorizationInfo.ResultInfo.Code == 0)
        {
            _callerAuthorization.AuthCode = AuthCode;
            _callerAuthorization.FundsTransferInfo = FundsTransferInfo;
        }
    }

_debitBalanceAmount = debitBalance;

Assigning this to a member value doesn't seem right, and this is before any validation.

GetFundAmountToTransfer();

This function is implies it will return something, but you don't assign it anywhere.  This is a code smell.

var result = GetProceedInstruction();
if (result == "1")
    return _callerAuthorization;

A meaningless string literal, not good.  Make it an enum with a good name.  Where did _callerAuthorization get assigned?

The control flow is not complicated and yet it's hard to read.  Try breaking it up into smaller functions.


Find intersection of rectangle with itself rotated Given \$w, h, \alpha\$ for a rectangle centered at origin with width \$w\$, height \$h\$ and itself rotated by \$\alpha\$ around origin clockwise, I wrote a code to find the intersection area.

  First I find all 4 corners of original and rotated rectangle, along with lines joining them.
  Then I find intersection with axis-parallel lines using getints
  Then I take all intersections in a clockwise/anti-clockwise order and find area using the co-ordinates of the polygon thus formed.

Source: https://codeforces.com/problemset/problem/280/A

import Data.Ord
import Data.List
import Control.Monad

main :: IO ()
main = do
  [w, h, a] <- map read . words <$> getLine
  let a' = (min a (180 - a)) * pi / 180
  let pts = [(w/2, h/2), (w/2, -h/2), (-w/2,-h/2), (-w/2,h/2)] :: [(Double, Double)]
  let pts' = map (rotate a') pts
  let lines = zip pts (tail $ cycle pts)
  let lines' = zip pts' (tail $ cycle pts')
  let ints = concatMap (getints lines') lines
  print . area . uniq $ sortBy (comparing theta) ints

uniq [] = []
uniq [x] = [x]
uniq (x:y:xs)
  | x == y = uniq (x:xs)
  | otherwise = x : uniq (y:xs)

rotate a (x, y) = (x * cos a - y * sin a, x * sin a + y * cos a)

theta (x, y) = atan2 y x

getints xs y = concatMap (\x -> getints' x y) xs

getints' l'@((x1', y1'), (x2', y2')) l@((x, y), (x', y'))
  | x == x' =
    let y'' = y1' + (x - x1') / (x2' - x1') * (y2' - y1')
    in if min y y' <= y'' && y'' <= max y y' then [(x, y'')] else []
  | y == y' = map swap $ getints' (swap' l') (swap' l)
  where
    swap (a, b) = (b, a)
    swap' (p1, p2) = (swap p1, swap p2)

area ps =
  let
    xs = map fst ps
    ys = map snd ps
  in
    0.5 * abs (sum $ zipWith4 (\x y y' x' -> x * y - y' * x') xs (tail $ cycle ys) ys (tail $ cycle xs))

Not a full rewrite, but some pointers:

Typically, a main function's do-block shouldn't be doing a whole lot of work. Additionally, do-blocks should try to avoid having lots of let statements, and instead "outsource" that work to helper functions. A function with type Double -> Double -> Double -> [((Double, Double), (Double, Double))] could replace most of the do block (the arguments would be w, h, and a).

Every style guide I've seen recommends putting type signatures on every top-level function. Just reading the code to try and figure out what the above mentioned type should be can be surprisingly difficult. Unfortunately, as the author of a program, it's really easy to forget that you know the types because the ideas for the program were your own.

Primes in function names are typically reserved for one of two things. It could mean a strict variant, such as foldl'. "Strict" in this case means that the function evaluates its argument, as opposed to Haskell's default of waiting to evaluate things as long as possible. The other option is that it's a minor variation on the function of the same name (many haskellers would say to avoid doing even this, and to reserve primes on function names for strict varaints alone). getints' is not a minor variation on getints, it is a helper function. A more appropriate name might be getintsHelper. On the other hand swap' is a minor variation on swap, and barring a better name like swapBoth, swap' is a reasonable name for that function.

Your function uniq looks like Data.List.nub. nub is a strange name and uniq honestly makes more sense, but it's better to use library functions that readers are familiar with. It's also fine to define synonyms, like uniq = nub, this way readers can see that in the source. Recognizing that uniq is a complete re-implementation is a bit harder. If you think a function you need is simple or common, check the standard library! Haskell's is quite extensive.

With top-level types added and no other changes, this would be a very strong starting point. I'd label that as good work!