Roll-a-ball controller I've been toying around with Unity 5 lately, and in an effort to start making an actual game, I've built a ball controller similar to the one found in the Unity tutorial Roll-a-ball.

In essence, the player controls a small ball that rolls around a "level". The ball can accelerate at a rate in the range \$1\rightarrow10\$ depending on what I (the designer) choose. The controller also manually calculates drag to create a "low-gravity" feel, but still have a sense of speed. Drag is calculated using the following formula:

$$\text{drag}=\frac{\text{acceleration vector magnitude}}{\text{some dividend}}$$

I'm wondering about the following things:

Is it appropriate for the controller to do things such as drag calculation, or should this be separated into a separate area?
I'm currently using a dictionary, looping through it, checking for keypresses, and then applying force if a key is pressed. Is this a good way to handle it? Will this ever slow anything down?
Are there any performance improvements that can be made?
Anything else?

BallController.cs

using UnityEngine;
using System.Collections.Generic;

/// <summary>
/// This class contains various methods and attributes
/// used to control the "vehicle".
/// </summary>
[System.Serializable]
[RequireComponent(typeof(Rigidbody))]
public class MoveVehicle : MonoBehaviour
{
    /// <summary>
    /// The force of an acceleration. This will be applied
    /// to the ball when it's accelerating.
    /// </summary>
    [Range(1, 10)]
    public float accelerationForce;

    /// <summary>
    /// This determines the drag being exerted on the ball
    /// as it moves. Drag is determined through the formula
    /// drag = accelerationForce / dragDividend
    /// </summary>
    [Range(0, 1000)]
    public float dragDividend;

    private Rigidbody rigidBody;
    private Dictionary<KeyCode, Vector3> keyMappings = new Dictionary<KeyCode, Vector3>() {
        {KeyCode.Space, new Vector3(0, 5.5f, 0)},

        {KeyCode.W, new Vector3(0, 0, 1)},
        {KeyCode.A, new Vector3(-1, 0, 0)},
        {KeyCode.S, new Vector3(0, 0, -1)},
        {KeyCode.D, new Vector3(1, 0, 0)},

        {KeyCode.UpArrow, new Vector3(0, 0, 1)},
        {KeyCode.LeftArrow, new Vector3(-1, 0, 0)},
        {KeyCode.DownArrow, new Vector3(0, 0, -1)},
        {KeyCode.RightArrow, new Vector3(1, 0, 0)}
    };

    /// <summary>
    /// Check if the ball is near to, or touching the
    /// ground. This is used to determine whether the
    /// vehicle can move or not.
    /// </summary>
    public bool IsGrounded()
    { return Physics.Raycast(this.transform.position, -Vector3.up, 0.65f); }

    /// <summary>
    /// Initialize things like our Rigidbody component,
    /// or other things that can only be initialized in
    /// Start.
    /// </summary>
    public void Start()
    { this.rigidBody = GetComponent<Rigidbody>(); }

    /// <summary>
    /// Apply thrust to the ball. The faster the vehicle
    /// gets, the more drag we apply to the rigidbody. This will
    /// create the effect of reaching a "maximum" speed.
    /// </summary>
    public void FixedUpdate()
    {
        foreach(KeyValuePair<KeyCode, Vector3> keyMapping in this.keyMappings)
        {
            if(Input.GetKey(keyMapping.Key) && this.IsGrounded())
            {
                Vector3 objectForce = keyMapping.Value * this.accelerationForce;
                this.rigidBody.drag = objectForce.sqrMagnitude / this.dragDividend;
                this.rigidBody.AddForce(objectForce);
            }
        }
    }
}

First let us nitpick a little bit.

MoveVehicle would be a nice name for a method but for a class it isn't choosen well. A class name should be made out of a noun or a noun phrase. So for instance VehicleMover or VehicleController would be a much better name for this class.

The method in question

public void FixedUpdate()
{
    foreach(KeyValuePair<KeyCode, Vector3> keyMapping in this.keyMappings)
    {
        if(Input.GetKey(keyMapping.Key) && this.IsGrounded())
        {
            Vector3 objectForce = keyMapping.Value * this.accelerationForce;
            this.rigidBody.drag = objectForce.sqrMagnitude / this.dragDividend;
            this.rigidBody.AddForce(objectForce);
        }
    }
}

here I would suggest to use var rather than KeyValuePair<KeyCode, Vector3> which would make your code a little bit cleaner and its obvious that it will be a KeyValuePair<TKey,TValue.

Otherwise your code looks nice and tidy. Good job.

I don't know Unity3D at all, so I don't know if it is possible to switch the public float accelerationForce; to a public property. If this is possible, I would suggest inside the setter to change all the values of the Dictionary so they represent the objectforce. This would involve a second kind of collection or some constants to hold the currently predifined values of the current Dictionary.

If it is likely that the accelerationForce is called less often than the FixedUpdate() method this will speed the whole thing up.


Run stored procedure from repository using EF I need to dynamically call a stored procedures from my repository, and here is a my implementation:

        public void ExecProcedure(string name, Dictionary<string, string> param = null)
        {
            List<object> sqlParametesList = new List<object>();

            try
            {
                if (param != null)
                {
                    name = NormalizeProcedureName(name, param);
                    foreach (var item in param)
                    {
                        sqlParametesList.Add(new SqlParameter(item.Key, item.Value));
                    }
                }

                context.Database.ExecuteSqlCommand(name, sqlParametesList.ToArray());
            }
            catch (Exception e)
            {
                throw new Exception(string.Format("Error in ExecProcedure: {0}", e.Message));
            }
        }

There is 3 case of call ExecProcedure():

_repo.ExecProcedure("usp_Test"); // call without parameters
_repo.ExecProcedure("usp_Test2 @id", param); // set parameter name after procedure name and pass value as param
_repo.ExecProcedure("usp_Test2", param); // if passing param then we use method NormalizeProcedureName() to set parameter name after procedure name

private string NormalizeProcedureName(string name, Dictionary<string, string> param)
{
    if (!name.Contains("@"))
    {
        StringBuilder sb = new StringBuilder();
        string delimiter = "";
        sb.Append(name);

        foreach (var item in param)
        {
            sb.Append(delimiter);
            sb.Append(" @");
            sb.Append(item.Key);
            delimiter = ",";
        }

        name = sb.ToString();
    }
    return name;
}

What do you think about this implementations?
Any advice will be appreciated. Thanks

Dictionary<string, string> param: I realize why you named it param and not params, but the real solution was to call it parameters.

Even worse as a name is sqlParametesList: it contains a typo, and the name also contains the type ("List") which is completely superfluous.

But the real problem is that a Dictionary<string, string> is too limited. What if you need to pass an int or a datetime for instance? You even acknowledge this by converting the strings to objects, which isn't a solution at all.

NormalizeProcedureName is a seriously bad idea. It doesn't even do what it says, and what it does can be done far simpler with a string.join() -- not that this would be a good idea either.

I don't get the point of this:

throw new Exception(string.Format("Error in ExecProcedure: {0}", e.Message));

The stack trace should tell you where the exception happened, but you throw it away to create a far simpler error message.

IMO it would be far better to create a custom class to hold the necessary parameter data: the parameter name, its value and its type. You then pass a list of these objects to ExecProcedure, and loop through the list to construct the parameters you need to pass to ExecuteSqlCommand, making sure to also include the correct SqlDbType.

Or perhaps you can simply use SqlParameter instead of a custom class, it depends a bit on how you've abstracted your db logic from your business logic.

EDIT: To provide some guidance, here's an example of how I handled a similar problem.

I had to call a stored proc in an Oracle DB, which required a number of parameters. So I created this class:

public class LogMessageParameters
{
    public DateTime? Date { get; set; }

    public string From { get; set; }

    public long? Code { get; set; }

    public List<OracleParameter> ToOracleParameters()
    {
        return new List<OracleParameter>
        {
            new OracleParameter("DATE", OracleDbType.Date, Date, ParameterDirection.Input),
            new OracleParameter("FROM", OracleDbType.VarChar, From, ParameterDirection.Input),
            new OracleParameter("CODE", OracleDbType.Number, Code, ParameterDirection.Input),
            new OracleParameter("ID", OracleDbType.Number, ParameterDirection.Output)
        };
    }
}

Now all I needed to do was assign the proper values to the properties, and pass this class to the method where I needed to use the parameters.

Is this the ideal way? Not if I had to create a dozen of these classes, no: then I'd made sure to use reflection to figure out the type of a property so I could return the proper OracleDbType, I'd have used an attribute on a property to store the parameter name, etc.


Super Market Pricing Model I have modelled the task of super market pricing as per the below task.
http://codekata.com/kata/kata01-supermarket-pricing/

Please review my code and suggest any improvements.

class Product
{
    String Name; //Name of the product
    int unit_price;
    UnitType unit_type;
    boolean groupOffer; //Is this product eligible for a group offer?
    int buy,get; //If so..what are the values of buy and get..

    public Product(String name, int unit_price, UnitType unit_type, boolean groupOffer) {
        // TODO Auto-generated method stub
        Name = name;
        this.unit_price = unit_price;
        this.unit_type = unit_type;
        this.groupOffer = groupOffer;
    }

    public Product(String name, int unit_price, UnitType unit_type,boolean groupOffer, int buy, int get) {
        this(name,unit_price,unit_type,groupOffer);
        this.buy = buy;
        this.get = get;
    }

}

enum UnitType
{
    NUMERIC, //to represent non-breakable units.
    POUND,
    KG,
    GRAM;
}

public class SuperMarket {
    public static void main(String args[])
    {
        //Configuring the price of products.
        Product pen = new Product("Pen",5,UnitType.NUMERIC,Boolean.FALSE);
        Product potato = new Product("Potato",2,UnitType.POUND,Boolean.FALSE);
        Product soda = new Product("Soda",1,UnitType.NUMERIC,Boolean.TRUE,4,1);
        //Calculating the prices
        System.out.println(getPrice(pen,4));
        System.out.println(getPrice(pen,4.1f)); //Pen cannot be a fraction
        System.out.println(getPrice(potato,3));
        System.out.println(getPrice(potato,3.1f));//Potato can be a fraction
        System.out.println(getPrice(soda,10)); // Eligible for group offer
    }

    //Get the price of products
    public static float getPrice(Product product,float quantity)
    {
        if(product.unit_type == UnitType.NUMERIC)
        {
           if(quantity != Math.round(quantity))
               return -1000;
        }
        float price = product.unit_price * quantity;
        //To decrease the price of free items.
        if(product.groupOffer)
        {
             price = (quantity/(product.buy+product.get)) * product.buy * product.unit_price;
        }
        return price;
    }
}

String Name; – Variable names begin with lowercase by convention: String name;.
unit_price; unit_type; – Variable names are camel case by convention: unitPrice; unitType;
boolean groupOffer – It's convention to start boolean variables with is..., has..., can..., must...: boolean isEligibleForGroupOffer. Such you also don't need the comment.
int unit_price;  int buy,get;

Your amounts are integers only? Really? Not float or BigDecimal:

  Currency

  It is recommended to use BigDecimal class while dealing with Currency or monetary values as it provides better handling of floating point numbers and their operations.

buy and get are verbs that fit better to a method name. Do you mean buyingPrice and sellingPrice?
More than one variable declaration per line is discouraged.

I'd rename UnitType to SellingUnitand NUMERIC to PIECES.
I'd implement the body of your first constructor with just:

this(name, unitPrice, sellingUnit, isEligibleForGroupOffer, 0, 0);

And the one of the second with:

this.name = name;
this.unitPrice = unitPrice;
this.sellingUnit = sellingUnit;
this.isEligibleForGroupOffer= isEligibleForGroupOffer;
this.buyingPrice = buyingPrice;
this.sellingPrice = sellingPrice;

I'd add a space after the commas in parameter and argument lists for easier reading.
I'd change static ... getPrice(...) to a non-static member of Product with then a header of public BigDecimal getPriceFor(float quantity).
It's convention to separate name and parenthesis in statements like if (...) with a space to easily distinguish them from method invocations.
I'd simplify:

float price = product.unit_price * quantity;
//To decrease the price of free items.
if(product.groupOffer) {
  price = (quantity/(product.buy+product.get)) * product.buy * product.unit_price;
}
return price;

to:

return isEligibleForGroupOffer  //To decrease the price of free items.
         ? quantity / (buyingPrice + sellingPrice) * buyingPrice * unitPrice
         : unitPrice * quantity;


Encryption/decryption by matrix multiplication in C I need to encrypt and decrypt text, image and video with a program in C with a matrix.

My program is working but it's really slow, how can I improve its speed ?
specially for the decrypt part.

Each matrix have an identity part and a filling part.

For encryption, I only do a matrix multiplication of each hex of ascii I get from files.

There is my code for decryption:

In source_decode I have the name of the file I need to decrypt.

In matrice_identity I have an array that contain the order of the matrix identity hidden in the matrix.
For example for a matrix like:

10001111
11000111
10100100
10010010

The matrice_identity will contain {4,1,2,3}, because the column 4 (where columns are numbered starting from 0) is 1000, the column 1 is 0100, column 2 is 0010, and column 3 is 0001. Other columns are just here to fill.

void decode_it(char* source_decode ,int* matrice_identity){

    FILE* source_file = NULL;
    source_file = fopen(source_decode, "rb");

    // that's to make difference between the original one, and the decrypted one.

    char* exetention = "d";
    strcat(source_decode, exetention);

    FILE* decoded_file = NULL;
    decoded_file = fopen(source_decode, "wb");

    if(decoded_file != NULL){
    int carac;

        if(source_file != NULL){
            int i = 0;
            int j = 0;
            int octet1[8] = {0};
            int octet2[8] = {0};
            int result[8] = {0};

            //encrypted file are writed in acsii, so here, i get an ascii number.
            carac = fgetc(source_file);

            while(feof(source_file) == 0){

                 //this function convert an ascii number to a byte.
                 convert_hex_bin_v2(carac, octet1);
                 carac = fgetc(source_file);
                 convert_hex_bin_v2(carac, octet2);

                 //This reason of why i'm getting 2 carac is beacause that I need to decrypt 2 bytes to have the original ascii.

                //For example a letter 'a' encrypted give me 2 bytes.

            for(j=0; j<4; j++){
                for(i=0; i<8; i++){
                    if(i == matrice_identity[j]){
                        result[j] = octet1[i];
                        result[j+4] = octet2[i];
                    }
                }
            }

            int k = 0;
            int printFile = 0;
            for(i=7; i>=0; i--){
                printFile += result[i] * pow(2.,(double)k);
                k++;
            }
            fprintf(decoded_file, "%c", printFile);

            carac = fgetc(source_file);
        }
        fclose(decoded_file);

        }else{
        printf("read failed");

        }

        fclose(source_file);

    }else{
        printf("output creation failed");
    }
}

And there is the function that convert hex to binary:

void convert_hex_bin_v2(int carac, int* octet){

        int x;
          for (x = 0; x < 8; x++) {
            octet[x] = 0;
          }

    if(carac >= 128){

     octet[0]=1;
     carac -=128;
    }
    if(carac>=64){

     octet[1]=1;
     carac -=64;
    }
    if(carac>=32){
     octet[2]=1;
     carac -=32;
    }
    if(carac>=16){
     octet[3]=1;
     carac -=16;
    }
    if(carac>=8){
     octet[4]=1;
     carac -=8;
    }
    if(carac>=4){
     octet[5]=1;
     carac -=4;
    }
    if(carac>=2){
     octet[6]=1;
     carac -=2;
    }
    if(carac>=1){
     octet[7]=1;
     carac -=1;
    }
}

Welcome to Code Review, nice first question!

Perform Error Checking as Soon as Possible
The complexity of the code can be decreased by performing earlier error checking in the function decode_it. If the code was changed so that source_file was tested at the beginning there would be less indentation
and the code would be easier to follow:

void decode_it(char* source_decode ,int* matrice_identity){

    FILE* source_file = NULL;
    source_file = fopen(source_decode, "rb");
    if (source_file == NULL)
    {
        printf("read failed");
        return;
    }

Don't Assume the Buffer has the Necesary Capacity
There is a possible critical error in the following code:

    char* exetention = "d";
    strcat(source_decode, exetention);

The variable source_code may not have enough space allocated to append the extention. This can cause the program to crash, especially when the code is optimized. Allocate a new string with enough space to accept the file name and the extension.

    char* decoded_fname = malloc(strlen(source_decode) + strlen(exetention) + 1);  // leave room for NULL termination
    strcpy(decoded_fname, source_decode);
    strcat(source_decode, exetention);

Reduce Complexity, Follow SRP

The Single Responsibility Principle states that every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.

Robert C. Martin expresses the principle as follows:

  A class should have only one reason to change.

While this is primarily targeted at classes in object oriented languages it applies to functions and subroutines in procedural languages like C as well.

The decode_it() function can be broken up into smaller functions to make it more readable, and to allow you to profile the program
to see where you are spending the most time.

The following is a good candidate for a function:

            for (j = 0; j<4; j++) {
                for (i = 0; i<8; i++) {
                    if (i == matrice_identity[j]) {
                        result[j] = octet1[i];
                        result[j + 4] = octet2[i];
                    }
                }
            }

Another good candidate for a function is the code

            int k = 0;
            int printFile = 0;
            for (i = 7; i >= 0; i--) {
                printFile += result[i] * (int)pow(2., (double)k);
                k++;
            }

Please note in the previous I cast the result of pow function to int, my compiler gave me a warning message on that line.

Possible Optimizations
Rather than reading the input file character by character input large chunks of data (file block size perferably) into a large
buffer internal to the program and process the buffer instead. Each time fgetc is called it is making a system call which is
expensive(time consuming). Having an input buffer and processing it may help reduce the complexity of the code as well.

For the same reason, create a string output buffer and write it to the file all at once rather than repeatedly calling fprintf().

Using pointers rather than array indexes may also improve performance.


PHP split string into uneven parts I am trying to split a string so that the first word goes into the first span, and the remaining (an unknown number of words) goes into the next.

This currently works fine, but is there an easy / faster / tidier way to do it ?

<?
// Explode the job role out and combine into the two spans

    $la_jobrole = explode(' ', $lo_designer->role);
    $ls_jobPrimary = '';
    $ls_jobSecondary = '';
    $i=0;
    foreach($la_jobrole as $role) {
        if ($i == 0) $ls_jobPrimary = $role;
        else {
            $ls_jobSecondary .= $role . " ";
        }
        $i++;
    }

?>

<?= $ls_jobPrimary ?> <?= $ls_jobSecondary ?>

explode and implode are faster (according to PHP's man page for preg_split), and more readable IMO.

$la_jobrole = explode(' ', $lo_designer->role);
$ls_jobPrimary = array_shift($la_jobrole); // Get the first item from the array
$ls_jobSecondary = implode(' ', $la_jobrole); // Glue the remaining pieces back together

Personally I would avoid using either list or preg_replace in this case. They're slower and harder to read, and I would certainly avoid that looping thing.


Angular filter to format temperature This a basic filter I created with Angular that does temperature conversion. I'd like to get thoughts on how to improve this code from both an Angular and general JS perspective.

The filter should take 2 arguments:

scale: The temperature scale to convert to
label: Whether to append a degree symbol to the number

In the view:

// should output: 99
{{ 37 | formatTemperature: 'F' }}

// should output: 99°
{{ 37 | formatTemperature: 'F':true }}

// should output: 37
{{ 98.6 | formatTemperature: 'C' }}

The filter:

(function() {
  'use strict';

  angular.module('example')
    .filter('formatTemperature', [
      function() {
        return function(input, scale, label) {
          var value = parseInt(input, 10),
              convertedValue;

          if (isNaN(value)) throw new Error('Input is not a number');

          if (scale === 'F') {
            convertedValue = Math.round(value * 9.0 / 5.0 + 32);
          } else if (scale === 'C') {
            convertedValue = Math.round((value - 32) * 5.0 / 9.0);
          } else {
            throw new Error('Not a valid scale');
          }

          return label ? convertedValue += '\u00B0' : convertedValue;
        };
      }
    ]);
})();

Testing the filter:

(function() {
  'use strict';

  describe('formatTemperature test', function() {
    beforeEach(module('example'));

    var filter;

    beforeEach(inject(function(_$filter_) {
      filter = _$filter_('formatTemperature');
    }));

    it('should convert temperature from Celsius to Fahrenheit', function() {
      expect(filter(37, 'F')).toEqual(99);
    });

    it('should convert temperature from Fahrenheit to Celsius', function() {
      expect(filter(98.6, 'C')).toEqual(37);
    });

    it('should append a % to the converted value', function() {
      expect(filter(37, 'F', true)).toEqual('99\u00B0');
    });

    it('should throw an error if the scale is invalid', function() {
      expect(function() {
        filter(37, 'G');
      }).toThrow(new Error('Not a valid scale'));
    });

    it('should throw an error if value is not a number', function() {
      expect(function() {
        filter('test', 'F');
      }).toThrow(new Error('Input is not a number'));
    });
  });

})();

On the principal that functions should do one thing and do it well, consider breaking out some pieces of your filter into smaller functions for readability.

Filter

(function () {
    'use strict';

    angular.module('example')
        .filter('formatTemperature', formatTemperatureFilter);

    var degreesSymbol = '\u00B0';

    function convertCelsiusToFahrenheit(value) {
        return Math.round(value * 9.0 / 5.0 + 32);
    }

    function convertFahrenheitToCelsius(value) {
        return Math.round((value - 32) * 5.0 / 9.0);
    }

    function addDegreesSymbol(value) {
        return value += degreesSymbol;
    }

    function formatTemperatureFilter() {
        return function (input, scale, label) {
            var value = parseInt(input, 10),
                convertedValue;

            if (isNaN(value)) throw new Error('Input is not a number');

            if (scale === 'F') {
                convertedValue = convertCelsiusToFahrenheit(value);
            } else if (scale === 'C') {
                convertedValue = convertFahrenheitToCelsius(value);
            } else {
                throw new Error('Not a valid scale');
            }

            return label ? addDegreesSymbol(convertedValue) : convertedValue;
        };
    }

})();

Make sure that your tests are testing one thing!

This will fail if you changed your conversion functions. Maybe you want to add 1 decimal place in the future.

it('should append a % to the converted value', function() {
      expect(filter(37, 'F', true)).toEqual('99\u00B0');
});

Change to a matcher that looks for the degrees symbol

it('should append a degrees symbol to the converted value', function() {
     expect(filter(37, 'F', true)).toContain('\u00B0');
});

Lastly, the only acceptable values for the scale are F and C, should this be case sensitive?


Queue Implementation using a Linked List For understanding the concepts, I've implemented the Queue data structures using a linked list. Is there anything to improve?

LinkList.java

public class LinkList {

    private static class Node<T> {

        private final T data;
        private Node next;

        public Node(T data){
            this.data=data;
        }

        public void displayNode(){
            System.out.print(data+ " ");
        }

    }

    private Node first = null;
    private Node last = null;

    public boolean isEmpty() {
        return (first == null);
    }

    public <T> void addLast(T data) {
        Node n = new Node(data);
        if (isEmpty()) {
            n.next = first;
            first = n;
            last = n;
        } else {
            last.next = n;
            last = n;
            last.next = null;
        }

    }

    public void removeFirst() {

            Node temp = first;
            if (first.next == null)
                last = null;
            first = first.next;

        }

    public void displayList() {
        Node current = first;
        while (current != null) {
            current.displayNode();
            current = current.next;
        }
    }

}

LinkListQueue.java

public class LinkListQueue {

    LinkList newLinkList = new LinkList();

    public <T> void enqueue(T data) {
        newLinkList.addLast(data);
    }

    public void dequeue() {
        if(!newLinkList.isEmpty())
            newLinkList.removeFirst();

    }

    public void displayQueue() {
        newLinkList.displayList();
        System.out.println();
    }

    public boolean isEmpty(){
        return newLinkList.isEmpty();
    }

}

LinkListQueueDemo.java

public class LinkListqueueDemo {

    public static void main(String[] args) {

        LinkListQueue queueImpl = new LinkListQueue();

        queueImpl.enqueue("A");
        queueImpl.enqueue("B");
        queueImpl.enqueue("C");
        queueImpl.enqueue("D");
        queueImpl.displayQueue();
        queueImpl.dequeue();
        queueImpl.displayQueue();

    }

}

BUG:

public void removeFirst() {

        Node temp = first;
        if (first.next == null)
            last = null;
        first = first.next;

    }

This crashes if you have an empty list. Add a check for first == null. Additionally... what's that temp variable for?

Don't define the Node as T, define the List to contain T.

public class LinkList<T>

This because you use the T outside the Node class and you ended up having to add it to the addLast method.

    if (isEmpty()) {
        n.next = first;
        first = n;
        last = n;
    } else {
        last.next = n;
        last = n;
        last.next = null;
    }

You already know that n.next is null. You also know that first is null, because that's what defines isEmpty().

So instead, try this:

    if (isEmpty()) {
        first = n;
    } else {
        last.next = n;
    }
    last = n;
    last.next = null;

It does the same thing.

Design:

You have an insert (enqueue) method... but there's no way to retrieve an object that went into the Queue. removeFirst could return the object. This drastically cuts down on the uses of such an object (it's useless now).


GAE datastore JOIN + GROUP BY I had posted this here and was asked to move it to code review since my code works and am looking for the best possible solution : original post

NOTE: I have seen the JOIN in GAE related posts but GROUP BY in my requirements made me make a new post and I have made a solution of my own which I wanted to check how good is it.

Datastore Entities properties and types (database)

Entity : PartyEntity let's call this as a

String partyName
String partyId
BlobKey image

Entity : InsertEntity let's call this as b

String partyIdentifier
String name
String constituency

Objective

SELECT * FROM a JOIN b on a.partyId = b.partyIdentifier GROUP BY b.constituency;

The solution I made is as follows.  Do suggest any changes or better ideas.

// SELECT * FROM a (PartyEntity holds unique partyId) and for each partyId SELECT * FROM b

private List<ListDisplay> initData() {

    DatastoreService datastore = DatastoreServiceFactory
            .getDatastoreService();

    List<ListDisplay> data = new ArrayList<ListDisplay>();

    Query query = new Query("PartyEntity");
    PreparedQuery preparedQuery = datastore.prepare(query);
    List<Entity> entities = preparedQuery.asList(FetchOptions.Builder
            .withDefaults());

    for (Entity entity : entities) {

        String PARTY_ID = (String) entity.getProperty("partyId");

        // fetch from another
        Query query2 = new Query("InsertEntity");
        Filter filterByPartyId = new Query.FilterPredicate(
                "partyIdentifier", FilterOperator.EQUAL, PARTY_ID);
        query2.setFilter(filterByPartyId);
        PreparedQuery preparedQuery2 = datastore.prepare(query2);

        List<Entity> entities2 = preparedQuery2.asList(FetchOptions.Builder
                    .withDefaults());

        for (Entity entity2 : entities2) {
            ListDisplay display = new ListDisplay();
            display.setPartyId((String) entity.getProperty("partyId"));
            display.setPartyName((String) entity.getProperty("partyName"));
            display.setImage((BlobKey) entity.getProperty("image"));
            display.setName((String) entity2.getProperty("name"));
            display.setConstituency((String) entity2
                    .getProperty("constituency"));
            data.add(display);
        } // end loop entity2

    } // end loop entity

    return data;
}

// GROUP BY constituency clause part

public HashMap<String, List<ListDisplay>> getData() {
    HashMap<String, List<ListDisplay>> data = new HashMap<>();
    List<ListDisplay> list = initData();

    for (ListDisplay d : list) {
        // if null add
        if (data.get(d.getConstituency()) == null) {
            List<ListDisplay> internal = new ArrayList<>();
            internal.add(d);
            data.put(d.getConstituency(), internal);
        }
        // else modify
        else {
            List<ListDisplay> hashlist = data.get(d.getConstituency());
            hashlist.add(d);
            data.remove(d.getConstituency());
            data.put(d.getConstituency(), hashlist);
        }
    }

    return data;
}

Just a few generic notes, I'm not familiar with Google App Engine:

HashMap<...> reference types could be Map<...>:

public Map<String, List<ListDisplay>> getData() {

See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces
I'd rename this variable to result to express its purpose:

List<ListDisplay> data = new ArrayList<ListDisplay>();

remove is unnecesary here, put will override it:

data.remove(d.getConstituency());
data.put(d.getConstituency(), hashlist);

Consider replacing the following with a Multimap:

// if null add
if (data.get(d.getConstituency()) == null) {
    List<ListDisplay> internal = new ArrayList<>();
    internal.add(d);
    data.put(d.getConstituency(), internal);
}
// else modify
else {
    List<ListDisplay> hashlist = data.get(d.getConstituency());
    hashlist.add(d);
    data.remove(d.getConstituency());
    data.put(d.getConstituency(), hashlist);
}

The following uses Google Guava's Multimap and much simpler:

final ArrayListMultimap<String, ListDisplay> data = ArrayListMultimap.create();

for (ListDisplay d : list) {
    data.put(d.getConstituency(), d);
}

If you really need Map return type you can convert it  back to Map with data.asMap() which returns a Map<String, Collection<ListDisplay>> but you cast it down safely to Map<String, List<ListDisplay>>. (ArrayListMultimap javadoc supports this as well as these Stack Overflow posts.)
Variable names in Java usually camelCase.

String PARTY_ID = (String) entity.getProperty("partyId");

Comments on the closing curly braces are unnecessary and disturbing. Modern IDEs and editors could show blocks.

    } // end loop entity2

“// …” comments at end of code block after } - good or bad?


PHP Class that handles all includes / required files In effort to streamline applicable PHP file include or require functions I thought that making a class to handle this might be a good idea.

Originally I had an includes/init.php file that included everything, I wanted to make this more dynamic to the specific page the user was on. For example, some pages require the Google maps script or custom Javascript that other pages don't need.

Here is a quick simplified example that doesn't address the dynamic include declarations yet:

class JIncludes {
    public function getClasses() {
        foreach (glob("includes/classes/*.php") as $filename) {
            require_once($filename);
        }
    }

    public function getCoreTop() {
        require_once('includes/core/head.php');
        require_once('includes/core/header.php');
    }

    public function getCoreBottom() {
        require_once('includes/core/footer.php');
    }
}

Is this even a good idea? Will creating this Class object cause more overhead then the alternative?

I have tested this with the following in my index.php:

$include = new JIncludes();
$include->getClasses();
$include->getCoreTop();

$db = new DB('local');
$db->prepare('String');

The source shows the html from my head.php file fine and my PDO DB class works as well. Am I over-complicating things?

Mixing

You are mixing 2 things:

class loading
"template" loading

You aren't over complicating things - you try to over simplify them.

Class loading

In your solution you are loading all classes if only (for example) 2 of them required to serve the request the loading of the other 52321 class is just a waste of time and resources.

Read about automatic class loading: Autoloading Classes

With this feature in your mind you can create a custom logic to load the necessary classes (name resolution, class location in file system etc.).

Template

The other thing what you want is loading your templates into the rendering process. This is hard to do it right, all depends on your "framework" solutions for example naming conventions, is your app frame an MVC based stuff etc.


Defining functions in Vigenere cipher I'm trying to learn the process of defining/calling a (somewhat) complex function.

I understand the main idea, and can do it on a small (simple) scale.

A simple example:

def multiply(x, y):
     return x*y

numb1 = 2
numb2 = 3

print(multiply(numb1, numb2))

Now what I'm trying to do is clean up my Vigenere cipher by defining the functions, and what's stumping me is what parameters to use and why. I don't want to change the (possibly poorly-written) code, however I just want to see how it would look after making use of defined functions.

message = input("Enter a message to encrypt:\n").upper().replace(" ", "")
print("Enter your encryption key ("+str(len(message)),"or less letters.): ")
key = input().upper().replace(" ", "")
div_times = int(int(len(message))/int(len(key)))
remainder = int(len(message))%int(len(key))
key_ring = (((key)*(div_times+1))[:-(len(key)-remainder)])
alph = 26

print("-------\n"
      "Message:  ",message,"\n"
      "Key:      ",key_ring,
      "\n-------")
mvalues = [0]*len(message)
kvalues = [0]*len(key_ring)
m_position = 0
k_position = 0

for letter in message:
    mvalues[m_position] = ord(letter)
    m_position += 1

for key in key_ring:
    num = alph - (int(ord("Z")) - int(ord(key)))
    kvalues[k_position] = num - 1
    k_position += 1

m_position = 0
k_position = 0
print("\nEncrypted message: ", end="")
for character in message:
    newletter_v = (mvalues[m_position] + kvalues[k_position])
    if newletter_v > ord("Z"):
        newletter_v -= 26
    elif newletter_v < ord("A"):
        newletter_v += 26
    print(chr(newletter_v)+"", end="")
    m_position += 1
    k_position += 1

I think you're making this a lot more complicated than it needs to be. Forgive my rusty python, but you should be able to do something like

def viginere_encrypt(message, key):
    length = len(message)
    mnum = [0]*length
    for i in range(0, length):
        mnum[i] = letter_to_number(message[i])
    knum = [0]*len(key)
    for i in range(0, len(key)):
        knum[i] = letter_to_number(key[i]
    result = [0]*length
    for i in range(0, length):
        result[i] = number_to_letter(mnum[i] + knum[i % len(key])
    return result

def letter_to_num(letter):
    return ord(letter) - ord("a")

def number_to_letter(number):
    return chr(number%alpha + ord("a"))

Using list comprehensions, this can be shortened to

def viginere_encrypt(message, key):
    mnum = [letter_to_number(x) for x in message]
    knum = [letter_to_number(x) for x in key]
    return [number_to_letter(mnum[i] + knum[i%len(key)])for i in range(0, len(message)]

or even further to

def viginere_cipher(message, key):
    return [number_to_letter(letter_to_number(message[i]) + letter_to_number(key[i % len(key)])) for i in range(0, len(message)])

though that's probably too mushed together by now.

Also, I don't know python too well; there's probably a better way to convert between "a-z" and 0-25.

Anyways, functions are a Very Good Idea. Output can then be separated from business logic, so if, say, you need to use the cipher elsewhere in your code, you're not tied to the terminal. It also allows for much easier testing.


Server which writes to mongo and makes http request This is simple server which basically just writes something to mongo after it accepts POST request, and then initiates a new HTTP request.
I would appreciate feedback especially if I am doing correctly error handling etc.
The code does its job though.

const MongoClient = require('mongodb').MongoClient;
const express = require('express')
const app = express()
var bodyParser = require('body-parser');
var http = require('http');

/* Connection URL */
const URL = 'mongodb://localhost:27017';

/* Database Name */
const DB_NAME = 'config-insert';

/* Handle to the connection */
let db = null;

/* Middlewares */
app.use(bodyParser.json());

/*
 * Use connect method to connect to the server
 */
MongoClient.connect(URL, function (err, client) {
    if (err) {
        throw new Error("Error connecting to mongo");
    }

    db = client.db(DB_NAME);

});

/*
 * Insert one document to mongo database.
 *
 * Parameters
 *   data - what to insert.
 *   successCallback - success callback.
 *
 */
function insertOne(data, successCallback) {

    if (!db) {
        throw new Error("There is no connection");

    }

    /* Insert a single document */
    db.collection('config-history').insertOne(data, {
        w: 'majority'
        , wtimeout: 10000
        , serializeFunctions: true
    }, function (err, r) {

        if (err) {
            throw new Error("Error inserting");

        }

        successCallback(r);

    });
}

/*
 * Handler for post request
 */
app.post('/', function (req, res) {

    let data = req.body;

    insertOne(JSON.parse(JSON.stringify(data)) /* use this otherwise mongo was modifying data variable */, function success() {

        let req = http.request({
            host: '10.1.1.41',
            path: `/category/${data.id}`,
            method: 'PUT',
            port: 9202
        }, function (response) {

            var resp = '';
            response.on('data', function (d) {
                resp += d;
            });
            response.on('end', function () {
                if (JSON.parse(resp)._shards.successful == 1) {
                    res.status(200).json({ status: "ok" })
                }

            });

        });

        req.on('error', (e) => {
            console.error(`problem with request: ${e.message}`);
        });

        req.write(JSON.stringify(data));
        req.end();

    })

})

/*
 * Listen for connections.
 */
app.listen(4000, () => console.log('Example app listening on port 4000!'))

Some comments:

Code is not formatted properly. There is a bunch of unnecessary whitespace, and there is some inconsistent use of var/let.
Most of the comments just add noise, and can be removed.
You're not taking advantage of promises or async/await (both for your expressjs route, the mongodb client, and for the outgoing http requests).
There is no general error handling middleware, or any other error handling in the express route.
The server accepts requests before the database is connected.
A side-effect of this is that the db global needs to be null-checked in every function that uses it.
It's hard to test
The naming is a bit lacking. From DB_NAME and other clues I gather this is about some sort of config, but other than that it's hard to tell

I did some quick refactoring (not tested):

const MongoClient = require('mongodb').MongoClient
const express = require('express')
const app = express()
const bodyParser = require('body-parser')
const rp = require('request-promise')
const URL = 'mongodb://localhost:27017'
const DB_NAME = 'config-insert'

let db = null
async function connectDb(){
    let client = await MongoClient.connect(URL)
    db = client.db(DB_NAME)
}

async function insertOne(db, data){
    data = JSON.parse(JSON.stringify(data)) // use this otherwise mongo was modifying data variable

    return db.collection('config-history').insertOne(data, {
        w: 'majority',
        wtimeout: 10000,
        serializeFunctions: true
    })
}

async function putCategory(data){
    let response = await rp({
        uri: `http://10.1.1.41/category/${data.id}`,
        method: 'PUT',
        port: 9202,
        body: data,
        json: true
    })

    if(response._shards.successful !== 1){
        throw new Error(`Could not put ${data.id}`)
    }
}

const promiseRoute = route => async (req, res, next) => {
    try{
        await route(req, res)
    }catch(err){
        next(err)
    }
}

app.use(bodyParser.json())

app.post('/', promiseRoute(async (req, res) => {
    let data = req.body

    await insertOne(db, data)
    await putCategory(data)

    res.status(200).json({status: 'ok'})
}))

app.use((err, req, res, next) => {
    console.error(err)
    res.status(500).json({status: 'not nok'})
})

connectDb()
    .then(() => {
        app.listen(4000, () => console.log('Example app listening on port 4000!'))
    })
    .catch(err => {
        console.log(err)
    })

What I have done is:

Format the code
Remove almost all the comments
Used async await instead of callbacks. Notice I have wrapped the expressjs route in a helper middleware that just passes any uncaught errors on to the general error handler. For mongodb, the driver already supports promises, and for outgoing http requests we have the request-promise library.
Added error handling middleware that ensures you get a proper response even if a request fails
Not starting the http server before the db is connected. Notice how the db null check is gone.
Injecting db into insertOne to allow for easier testing in the future.
Turned the _shards.successful response into a more general error case since it was not handled anyway.
Moved the json-parse trick closer to where it is needed (you could probably use an Object.assign here as well if you wanted)


Return the opposite of two known strings given one This is probably silly, but I feel like there should be a much more elegant way to do this simple thing. I have two known strings, whichever one is passed in in the parameter I want to return the opposite. Current code:

def matching_sub(subscription_type)
  if subscription_type == "maths"
    "reading"
  else
    "maths"
  end
end

I came up with:

def matching_sub(subscription_type)
  (%w{reading maths} - [subscription_type]).first
end

and my colleague did:

def matching_sub(subscription_type)
  {"maths" => "reading",  "reading" => "maths"}[subscription_type]
end

Can anyone top that?

Your first two solutions both suffer when given unexpected input.  The first is biased towards returning "maths" as a default; the second tends to return "reading".

In contrast, your colleague's function returns nil, which is probably more appropriate.  I quite like that straightforward lookup, with a minor drawback being that each string is hard-coded twice.

Here's one more suggestion.  This one crashes with TypeError: nil can't be coerced into Fixnum when given invalid input.  You should decide whether that exception is beneficial or not for your purpose.

SUBJECTS = %w(reading maths)

def matching_sub(subscription_type)
  SUBJECTS[1 - SUBJECTS.index(subscription_type)]
end


addEventListener after fetched data This is working code, but I want to simplify, correct, and improve the code.

window.addEvent = function(elem,type,callback) {
    var evt = function(e) {
        return callback.call(elem,e);
    };
    var cb = function(e) { return evt(e); };
    elem.addEventListener(type,cb,false);
    return elem;
};
window.findParent = function(child,filter,root) {
    do {
        if( filter(child)) return child;
        if( root && child == root) return false;
    } while(child = child.parentNode);
    return false;
};
function on(type, target, callback) {
  window.addEvent(document.body, type, function(e) {
    var s = window.findParent(e.srcElement || e.target, function(elm) {
        return elm.classList.contains(target);
    },this);

    if( s && callback ) {
        callback(e)
    }
  });
}
on("click", "page-link", function(e){
 console.log("hey its working");
});

In this example, I update the pagination with fetch, and I have to bind addEventlistener to every page link. I am thinking about, If its there a cleaner way, using the latest techniques?  (with pure Javascript)

I think you can use querySelectorAll() - for example:

function on (type, target, callback) {
  document.querySelectorAll(target).forEach(function (elem) {
     elem.addEventListener(type, callback);
  })
}

Your function findParent seems similar to element.closest() . In case you need it to work with IE, you can use the code below. It works with a selector, not just a class name.

closest = (element, selector) => {
    while(element) {
        //For IE11
        if (!element.matches)
            element.matches = element.msMatchesSelector;

        if (element.matches(selector))
            return element;

        element = element.parentElement;
    }
}


Dijkstra's and Prim's algorithms: correctness/efficiency This is in preparation for a competition that only allows C++03.

// Neighbor index plus weight of edge connecting this vertex with neighbor
struct NeighborWeight
{
    Index neighbor;
    Distance weight;
};
// Represents adjacency information for a single vertex (contains all
// neighbors and weights of edges connecting them)
typedef std::list<NeighborWeight> AdjVertex;
// Represents adjacency information for all vertices (thus representing
// a complete directed/undirected graph)
typedef std::vector<AdjVertex> AdjList;

// Dijkstra's algorithm: computes SHORTEST PATH to all vertices from a starting
// vertex for a DIRECTED/UNDIRECTED graph with NONNEGATIVE edge weights.
// result.dist[v] = shortest distance from source to v.
// result.prev[v] = predecessor of v in shortest path from source to v.
// If v is not connected to start_vertex, dist[v] = max value of Distance, and
// prev[v] = -1.
// If v = start_vertex, prev[v] = -1.

// Prim's algorithm (call with do_prim = true): computes MINIMUM SPANNING TREE
// starting from a seed vertex for an UNDIRECTED graph with ANY edge weights.
// result.dist[v] should be ignored.
// result.prev[v] = parent of v in minimal spanning tree.
// If v is not connected to start_vertex, prev[v] = -1.
// If v = start_vertex, prev[v] = -1;

struct DijkstraResult
{
    std::vector<Distance> dist;
    std::vector<Index> prev;
};

DijkstraResult dijkstra(AdjList al, Index start_vertex, bool do_prim = false)
{
    typedef std::pair<Distance, Index> DistAndIndex;

    const Distance infinity = std::numeric_limits<Distance>::max();
    const Index undefined_index = -1;
    const Index num_vertices = al.size();

    std::vector<Distance> dist(num_vertices, infinity); // tentative distances
    std::vector<Index> prev(num_vertices, undefined_index); // predecessors
    std::vector<uint8_t> is_visited(num_vertices, false);
    std::set<DistAndIndex> unvisited_queue;

    dist[start_vertex] = 0;

    // Populate unvisited_queue in order with (Distance, Index) pairs
    unvisited_queue.insert(std::make_pair(0, start_vertex));
    for (Index v = 0; v < num_vertices; ++v)
        if (v != start_vertex)
            unvisited_queue.insert(unvisited_queue.end(),
            std::make_pair(infinity, v));

    while (!unvisited_queue.empty())
    {
        // u = index of unvisited vertex with least distance
        Index u = unvisited_queue.begin()->second;
        // If this least distance is infinity, break (no more connected verts)
        if (unvisited_queue.begin()->first == infinity) break;
        // Visit this vertex
        unvisited_queue.erase(unvisited_queue.begin());
        is_visited[u] = true;
        // In Prim's, distances are computed from the MST. Adding a vertex
        // to the MST makes the distance 0.
        if (do_prim) dist[u] = 0;
        // vert = vertex corresponding to u
        AdjVertex &vert = al[u];
        // Iterate through neighbors of u
        for (AdjVertex::iterator it = vert.begin(); it != vert.end(); ++it)
        {
            // v = it->neighbor is the index of the neighbor; it->weight is the
            // edge weight between u and v
            Index v = it->neighbor;
            // Skip if v is visited
            if (is_visited[v]) continue;
            // Now compute new dist for v
            Distance alt = dist[u] + it->weight;
            // And compare with previous
            if (alt < dist[v])
            {
                unvisited_queue.erase(std::make_pair(dist[v], v));
                unvisited_queue.insert(std::make_pair(alt, v));
                dist[v] = alt;
                prev[v] = u;
            }
        }
    }

    DijkstraResult res = { dist, prev };
    return res;
}

There's another set of typedefs and functions that duplicates a list of undirected edges into an AdjList structure.

I'll address some minor things (unrelated to efficiency, at least):

The comments describing the algorithm could be at the very top so that the code in between isn't missed.  Any comments describing the functions or the typedefs can stay above them.
These structs are in global scope, so they should be contained by something (such as a function) to put them into local scope, preventing unknown modifications.

For instance, DijkstraResult could probably be defined in the calling code, which I assume is main() (not shown here).  Wherever the result of dijkstra() is received, that struct should be contained there.  This will also help with keeping code lower in scope, which eases maintenance and prevents bugs associated with this.
If you still want a typedef for the std::pair, at least consider a less-obvious (and possibly more accurate) name.  The name DistAndIndex can still be read from the std::pair, so it doesn't help much.  It also doesn't save very many keystrokes, if that's even a concern.
The name infinity doesn't make sense here as you're not assigning it to anything considered infinite (it's also not a number, but a concept).  It is assigned to a max value, which is finite, also according to the documentation.

Bearing that in mind, a more appropriate name may be something like maxDistance.
Curly braces should be used here:

for (Index v = 0; v < num_vertices; ++v)
    if (v != start_vertex)
        unvisited_queue.insert(unvisited_queue.end(),
        std::make_pair(infinity, v));

From a readability standpoint, it's not entirely clear what is contained in what.  It can also hurt maintainability as adding or removing code from here can break something.

Regarding the code within the if block, the second line should be indented so that it is not read as a separate statement.

for (Index v = 0; v < num_vertices; ++v)
{
    if (v != start_vertex)
    {
        unvisited_queue.insert(unvisited_queue.end(),
            std::make_pair(infinity, v));
    }
}

uint8_t should be std::uint8_t in C++.


"Can you answer these queries I" challenge I have tried to solve the following problem on SPOJ.com:

  You are given a sequence

A[1], A[2], ..., A[N] . ( |A[i]| ≤ 15007 , 1 ≤ N ≤ 50000 )

  A query is defined as follows:

Query(x,y) = Max { a[i]+a[i+1]+...+a[j] ; x ≤ i ≤ j ≤ y }

  Given M queries, your program must output the results of these queries.

When I run it, I get a time limit error, but I don't know why.

import java.awt.Point;
import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.lang.reflect.Array;
import java.math.BigInteger;
import java.sql.PreparedStatement;
import java.text.Collator;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.Queue;
import java.util.Scanner;
import java.util.Stack;
import java.util.StringTokenizer;

 class TestClass {

static int[] dist ;
static int[][][] vis;
static ArrayList<Integer>[] resu;
static char[][] graph;
static int max;
static int min;
static int[] dp;
static int zz;
static int zzz;
static nodes[] som;
static nodes temp;
static boolean fin;

static class nodes{

      int sum ;
      int presum;
      int suffsum;
      int max;

      public nodes(){

      }
      public nodes(int sum , int presum , int suffsum , int max){

            this.sum = sum;
            this.presum = presum;
            this.suffsum = suffsum;
            this.max = max;
      }
}

public static nodes merge(nodes a , nodes b){

    nodes result = new nodes();

    result.sum =  a.sum + b.sum;
    result.presum = Math.max(a.presum, a.sum + b.presum);
    result.suffsum = Math.max(b.suffsum, b.sum+a.suffsum);
    result.max  = Math.max(Math.max(a.sum, b.sum), a.suffsum+b.presum);

    return result;

}

public static void maketree(int current ,int a , int b ,int[] arr){
//  System.out.println(a+" "+b+" "+current);
     if(a>b) return;

     if(a==b)
     {
         int x = arr[a];
         nodes newnode = new nodes(x,x,x,x);
         som[current] = newnode;
         return;
     }

     maketree(2*current, a, (a+b)/2, arr);
     maketree(2*current+1, (a+b)/2+1, b, arr);

     som[current] = merge(som[2*current], som[2*current+1]);

}

public static nodes query(int current , int a, int b, int i , int j){

     if(a==i && b==j) return som[current];

     int mid = (a+b)/2;

     if(j<=mid) return query(2*current, a, (a+b)/2, i, j);
     else if(i>mid) return query(2*current+1, (a+b)/2+1, b, i, j);
     else{

         return merge(query(2*current, a, (a+b)/2, i, j), query(2*current+1, (a+b)/2+1, b, i, j));
     }
}

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

Scanner in = new Scanner(new InputStreamReader(System.in));

int n = in.nextInt();
int[] a = new int[n+1];
som = new nodes[10000000];
for(int i=1;i<=n;i++) a[i]=in.nextInt();

temp = new nodes(Integer.MIN_VALUE,Integer.MIN_VALUE,Integer.MIN_VALUE,Integer.MIN_VALUE);

maketree(1, 1, n, a);

int Q = in.nextInt();

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

    int l =in.nextInt();
    int r =in.nextInt();
    System.out.println(query(1, 1, n, l, r).max);
    //System.out.println(som[2].max+ " "+ som[4].max +" "+ som[5].max);
}

}
 }

static int[] dist ;
static int[][][] vis;
static ArrayList<Integer>[] resu;
static char[][] graph;
static int max;
static int min;
static int[] dp;
static int zz;
static int zzz;
static nodes[] som;
static nodes temp;
static boolean fin;

Your variable names are very hard to understand. What's dist? Distance? Then why is it an array? What's vis? Visuals? Why is it a 3D array?

Make your variable names clearer, and add comments what it does.

Also, there is inconsistent spacing. You do stuff like:

static int[] dist ;

and you also do:

static int[][][] vis;

Be consistent. The conventions are "no space before ;", so this:

static int[][][] vis;

is correct. Also:

static class nodes{

Standard conventions for class names are PascalCase, not camelCase or alllowercase.

Also, I think all your variables need some breathing space:

if(j<=mid) return query(2*current, a, (a+b)/2, i, j);

should be:

if(j <= mid) return query(2 * current, a, (a + b) / 2, i, j);

Also, your formatting seems messed up. You have extra spaces everywhere, so try removing them.


Writing a better alternative to jQuery Autocomplete Over the last months I have been writing a jQuery plugin called Better Autocomplete (Code on Github). It originated from another project, a Drupal module called Linkit, but I decided it should be a standalone plugin. I have spent a lot of time refactoring the code, mainly to make it more generic and flexible for multiple purposes. I have now reached a point where it makes sense to actually ask for advice on the code, hopefully by professional, more experienced JavaScript developers.

EDIT: I read in the FAQ that I'm supposed to include some of the code. This is a shorter version of the code, describing the structure of the code. There is also documentation available.

(function ($) {

$.fn.betterAutocomplete = function(method) {

  var methods = {
    init: function(resource, options, callbacks) {
      var $input = $(this),
        bac = new BetterAutocomplete($input, resource, options, callbacks);
      $input.data('better-autocomplete', bac);
      bac.enable();
    },
    enable: function(bac) {
      bac.enable();
    },
    disable: function(bac) {
      bac.disable();
    },
    destroy: function(bac) {
      bac.destroy();
    }
  };

  var args = Array.prototype.slice.call(arguments, 1);

  // Method calling logic
  this.filter(':input[type=text]').each(function() {
    switch (method) {
        // ... Code here
    }
  });

  // Maintain chainability
  return this;
};

// This is the constructor function. Instantiated by using the new keyword
var BetterAutocomplete = function($input, resource, options, callbacks) {

  options = $.extend({
    charLimit: 3,
    // More options
  }, options);

  /**
   * These callbacks are supposed to be overridden by you when you need
   * customization of the default behavior. When you are overriding a callback
   * function, it is a good idea to copy the source code from the default
   * callback function, as a skeleton.
   */
  callbacks = $.extend({

    /**
     * Gets fired when the user selects a result by clicking or using the
     * keyboard to select an element.
     *
     *

Default behavior: Simply blurs the input field.
     *
     * @param {Object} result
     *   The result object that was selected.
     */
    select: function(result) {
      $input.blur();
    },
    // ... More callbacks for fetching data, parsing results etc.
  }

  var self = this,
    lastRenderedQuery = '',
    results = {}, // Caching database of search results.
    // ... More private instance variables

  // Create the DOM elements necessary and attach eventhandlers
  var $wrapper = $('
')
      .addClass('better-autocomplete')
      .insertAfter($input);

  // Public methods
  this.enable = function() { ... };

  // Then private methods
  var fetchResults = function() { ... };

  var getHighlighted = function() { ... };
};

}(jQuery);

Some hands on questions:

Does my design patterns look good? I am talking about modularity, namespacing, closures etc.
How can I stand out more from jQuery UI autocomplete and other existing plugins? What can I provide that they can't? Also, where can I reach the people who need this plugin?
Documentation. I have been using JSDoc toolkit but it is sometimes hard to understand. I have a few warnings when I generate the docs. Also I'm unsure about if I have correctly labeled functions, constructors etc. Can I improve it?

I can take criticism, so don't be shy, rather be honest.

You should strongly consider using a JS templating language such as Handlebars.js when rendering the interface.

  // Add the group if it doesn't exist
  var group = callbacks.getGroup(result);
  if ($.type(group) == 'string' && !groups[group]) {
    var $groupHeading = $('<li />').addClass('group')
      .append($('<h3 />').html(group))
      .appendTo($results);
    groups[group] = $groupHeading;
  }

  var $result = $('<li />').addClass('result')
    .append(output)
    .data('result', result) // Store the result object on this DOM element
    .addClass(result.addClass);

You are doing enough dom manipulation to warrant its use (above is just one part of your code). Furthermore, templates will help separate presentation logic from your business logic which at the moment is quite tangled.


SICP - exercise 1.12 - pascal's triangle From SICP:

Exercise 1.12: The following pattern of numbers is called Pascal’s triangle.

         1
       1   1
     1   2   1
   1   3   3   1
 1   4   6   4   1
       . . .

The numbers at the edge of the triangle are all 1, and each number inside the triangle is the sum of the two numbers above it. Write a procedure that computes elements of Pascal’s triangle by means of a recursive process.

Here is my code:

(define (pascals-triangle row col)
  ;; "The numbers at the edge of the triangle are all 1"
  (if (or (= col 0) (= col row))
      1
      (+ (pascals-triangle (- row 1)
                            (- col 1))
          (pascals-triangle (- row 1)
                            col))))

I noticed that the edges of the triangle after 1 is (probably) always the number of rows, because we always add 1 to them when we go to the next row. So I rewrote the code with a minor change.

(define (pascals-triangle row col)
  (cond ((or (= col 0) (= col row))
         1)
        ((or (= col 1) (= col (- row 1)))
         row)
      (else (+ (pascals-triangle (- row 1)
                                  (- col 1))
                (pascals-triangle (- row 1)
                                  col)))))

In my previous code, if the colum is 0, the running time is constant. Now in the revised code, it's also constant if the column is 1. And since this is recursive, I am guessing I saved a few steps. Will making small changes like that have a significant impact with my code and systems a lot larger than this?

How can I improve this code?

You should memoise the recursive call in order to make it faster. Since each value has a computational sibling (its value is derived from (one or two) parents which also each has another child), you remove the double-computation that would occur.

Additionally, these double-computations would actually accumulate exponentially up the levels, so it's no longer just a micro-optimisation; it actually affects the your runtime complexity. So it's even more important to implement memoisation.


Modifying a data table of report data Is there any way of optimizing this code?

private void ModifyDataTable(DataTable dt)
{
    if (dt != null)
    {
        bool b_dataValue = false;
        decimal rowreport_data0Sum, rowreport_data1Sum, rowreport_data2Sum, rowreport_data3Sum, rowreport_data4Sum;
        rowreport_data0Sum = rowreport_data1Sum = rowreport_data2Sum = rowreport_data3Sum = rowreport_data4Sum = 0;
        foreach (DataRow row in dt.Rows)
        {

            if (row.ItemArray[1].ToString() == "$150,000 & Over" || row.ItemArray[1].ToString() == "$150,000 - $199,999" || row.ItemArray[1].ToString() == "$200,000 - $299,999" || row.ItemArray[1].ToString() == "$300,000 & Over")
            {

                rowreport_data0Sum += GetRowValue(row, "report_data0");
                rowreport_data1Sum += GetRowValue(row, "report_data1");
                rowreport_data2Sum += GetRowValue(row, "report_data2");
                rowreport_data3Sum += GetRowValue(row, "report_data3");
                rowreport_data4Sum += GetRowValue(row, "report_data4");
            }
        }
        if (dt.Columns.Count > 0)
        {
            foreach (DataRow row in dt.Rows)
            {

                if (row.ItemArray[1].ToString() == "$150,000 & Over")
                {
                    foreach (DataColumn col in dt.Columns)
                    {
                        if ((col.ColumnName == "report_data0") || (col.ColumnName == "report_data1") || (col.ColumnName == "report_data2") || (col.ColumnName == "report_data3") || (col.ColumnName == "report_data4"))
                        {

                            if (!row.IsNull(col))
                            {
                                b_dataValue = true;
                                if (b_dataValue)
                                {
                                    if (col.ColumnName == "report_data0") row[col] = rowreport_data0Sum + "%";
                                    else if (col.ColumnName == "report_data1") row[col] = rowreport_data1Sum + "%";
                                    else if (col.ColumnName == "report_data2") row[col] = rowreport_data2Sum + "%";
                                    else if (col.ColumnName == "report_data3") row[col] = rowreport_data3Sum + "%";
                                    else if (col.ColumnName == "report_data4") row[col] = rowreport_data4Sum + "%";
                                }
                                else
                                {

                                    dt.Rows.Remove(row);

                                }
                            }
                        }
                    }

                }
            }
            if (b_dataValue)
            {
                for (int i = dt.Rows.Count - 1; i >= 0; i--)
                {
                    DataRow row = dt.Rows[i];
                    if (row.ItemArray[1].ToString() == "$150,000 - $199,999" || row.ItemArray[1].ToString() == "$200,000 - $299,999" || row.ItemArray[1].ToString() == "$300,000 & Over")
                    {
                        dt.Rows.Remove(row);
                    }
                }

            }
        }
    }
}

is there any possibility of optimizing the above code

Eliminating unnecessary processing will help...

Remove Needless if

if (!row.IsNull(col))
{
    b_dataValue = true;
    if (b_dataValue)
    {
        // ...
    }
    else
    {
        dt.Rows.Remove(row);
    }
}

b_dataValue is always true, so the if is not necessary, and the else will never execute. So ...

if (!row.IsNull(col))
{
    b_dataValue = true;
    {
        // ...
    }
}

Move If's to the top

If the table is null or has no columns then there's no point in going on. So check for this up front. This removes 2 nesting levels.

private void ModifyDataTable(DataTable dt) {
    if (dt == null) return;
    if (dt.Columns.Count <= 0) return;
    // ...
}

Reference by Table Name and Remove more If-ing

I'm guessing that your code is handling at least 2 different tables. If this is true then based on the name call "thisTable" code or "thatTable" code. Then you can eliminate all that checking for column names because you know they are there. And it eliminates another layer of if

private void ModifyDataTable(DataTable dt) {
    if (dt == null) return;
    if (dt.Columns.Count <= 0) return;
    if (dt.TableName == "thisTable") DoThisTableStuff(dt);
    if (dt.TableName == "thatTable") DoThatTableStuff(dt);

    // Knowing what table we're dealing with, code like this
   if (col.ColumnName == "report_data0") row[col] = rowreport_data0Sum + "%";
   else if (col.ColumnName == "report_data1") row[col] = rowreport_data1Sum + "%";

   // could turn into this
   row["report_data0"] = rowreport_data0Sum + "%";
   row["report_data1"] = rowreport_data1Sum + "%";

}


Extending Mojo::Promise First time poster, be gentle.

I wanted something similar to Mojo::Promise->all, except with an option to discard certain rejected (or, I guess, resolved) promises dependent on rejection value, as determined by caller.

Only using this internally, so nevermind my polluting the Mojo::Promise:: namespace.

package Mojo::Promise::Filter;
use Moose;

extends 'Mojo::Promise';

has 'promises' => (
    isa => 'ArrayRef[Mojo::Promise]',
    is => 'ro',
    required => 1,
);

has 'resolve_filter' => ( # truthy return means *DO* filter out
    isa => 'CodeRef',
    is => 'ro',
    required => 1,
    lazy => 1,
    default => sub { return sub {}; },
);

has 'reject_filter' => ( # truthy return means *DO* filter out
    isa => 'CodeRef',
    is => 'ro',
    required => 1,
    lazy => 1,
    default => sub { return sub {}; },
);

has '_promises_state_tracker' => (
    # numbered hashref whose primary keys correspond to passed promise-array's indices
    # secondary keys are flags for whether each original promise filtered
    # and whether promise resolved/rejected,
    # plus the original promise itself, e.g. :
    # { 0 => { filtered => (undef|1), ror => (undef|1), original_promise => (Mojo::Promise) } }
    isa => 'HashRef[HashRef]',
    is => 'ro',
    lazy => 1,
    builder => '_build_promises_state_tracker',
);

sub _build_promises_state_tracker {
    my $self = shift;
    my $i = 0;
    return { map { $i++ => { original_promise => $_ } } @{$self->promises} };
}

sub _resolve_or_reject_self_if_last_child {
    my $self = shift;

    my @ror = grep { $_->{ror} } values %{$self->_promises_state_tracker};
        # gather the thus-far resolved-or-rejected promises
    return unless scalar keys %{$self->_promises_state_tracker} == scalar @ror;
        # return unless we have as many ror promises as were passed in

    my @unfiltered = map { $_->{original_promise} } grep { !$_->{filtered} } values %{$self->_promises_state_tracker};
    return Mojo::Promise->all(@unfiltered)->then(sub {
        $self->resolve(@_)
    })->catch(sub {
        $self->reject(@_)
    });
}

sub filter {
    my $self = shift;

    for my $i (0 .. $#{$self->promises}) {
        $self->promises->[$i]->then(sub {
            $self->_promises_state_tracker->{$i}->{filtered} = 1
                if $self->resolve_filter->(@_);
        })->catch(sub {
            $self->_promises_state_tracker->{$i}->{filtered} = 1
                if $self->reject_filter->(@_);
        })->finally(sub {
            $self->_promises_state_tracker->{$i}->{ror} = 1;
                # ror = Resolved Or Rejected
            $self->_resolve_or_reject_self_if_last_child();
        });
    }

    return $self;
}

no Moose;

1;

Use as :

my $re_filter = qr/some special text/;
my $reject_filter = sub {
        my $v = shift;
        return 1 if ref \$v eq 'SCALAR' && $v =~ $re_filter;
    };

my $p = Mojo::Promise::Filter->new(
        reject_filter => $reject_filter,
        promises => \@promises
    );

return $p->filter;

I'm not entirely sure I understand what this does, but here goes.

Mojolicious doesn't use Moose. I would think long and hard whether I'd extend a Mojo::Base class with Moose. Does this work properly?
There are several instances of required together with default and lazy in your new attributes. Those do not make sense together. If it's required it needs to be passed in at construction. There is no need to make it lazy then as it will always be there from the start.
Your default for both reject_filter as well as resolve_filter are subs that return undef implicitly. That means if you don't pass those in, everything is discarded. Is that the intended behavior?
Your class is a subclass of Mojo::Promise, so there is no need to call Mojo::Promise->all. It's got an inherited ->all method that you can call on $self. (Mojo::Promise->all) returns a new object, so this breaks.


heapifyDown in heapsort This is for heapsort and am wondering about the heapifyDown method. Here it is for review. I included relevant helper methods.

The heap is stored in array starting at index 1.

/*swaps node at i with biggest child if larger*/
private void heapifyDown(final int i) {
    while(!isLeaf(i)) {
        //find biggest child
        int biggestChldIndx;
        if(getRgtChldIndx(i) <= end) {//check if node has right child
            if(heap[getLftChldIndx(i)] > heap[getRgtChldIndx(i)]) {
                biggestChldIndx = getLftChldIndx(i);
            } else {
                biggestChldIndx = getRgtChldIndx(i);
            }
        } else {//only has left child so use it
            biggestChldIndx = getLftChldIndx(i);
        }
        //check if child is larger than parent
        if(heap[i] < heap[biggestChldIndx]) {
            //if it is, then swap
            swap(i, biggestChldIndx);
            i = biggestChldIndx;
        } else {
            break;
        }
    }
}

private boolean isLeaf(final int position) {
    return (2 * position > end);
}

private int getLftChldIndx(final int i) {
    return (i * 2);
}

private int getRgtChldIndx(final int i) {
    return (i * 2 + 1);
}

private final int[] heap;
private int end;//last used element

int biggestChldIndx;
        if(getRgtChldIndx(i) <= end) {//check if node has right child
            if(heap[getLftChldIndx(i)] > heap[getRgtChldIndx(i)]) {
                biggestChldIndx = getLftChldIndx(i);
            } else {
                biggestChldIndx = getRgtChldIndx(i);
            }
        } else {//only has left child so use it
            biggestChldIndx = getLftChldIndx(i);
        }

You can simplify this somewhat.

        int biggerChildIndex = getLftChldIndx(i);
        int rightChildIndex = getRgtChildIndx(i);
        if (rightChildIndex <= end && heap[biggerChildIndex] <= heap[rightChildIndex]) {
            biggerChldIndx = rightChildIndex;
        }

The compiler would probably do most of that.  The basic point is that there are only two possible values for biggerChildIndex.  Since we need to fetch both values to pick the right one, we might as well assign one of them optimistically.  Then we only have to check for the one path where it would be the other value.

I renamed it to biggerChildIndex because when you have two things, one is bigger than the other.  Biggest implies that there are at least three.  If interested, look up the difference between comparatives (e.g. bigger) and superlatives (e.g. biggest).  A rather pedantic difference, but one that I happen to know.

I also prefer not to elide out letters.  The minor savings in typing are outweighed by the cognitive increase in reading in my opinion.  Since most code is read more than it is written, it makes more sense to optimize for reading.

You also might want to consider this pattern:

        for (int childIndex = getBiggerChildIndex(i); childIndex <= end && heap[i] < heap[childIndex]; childIndex = getBiggerChildIndex(i)) {
            swap(i, childIndex);
            i = childIndex;
        }

Some people prefer to iterate (which is what is happening here) with for loops rather than while loops.  In this case, it simplifies the logic somewhat by allowing us to combine the two iteration checks.  Previously you had to use a break because the second check was separated.

What you really want to know is that the child is inside the heap, not that i doesn't point to a leaf.  The other logic works because when i is a leaf, there is no child (and therefore the loop has to stop iterating).

An argument against this is that it adds a method call to each iteration of the loop.  Of course, the method might be inlined.  As I suspect that the calls to getLftChldIndx and getRgtChldIndx are.  Note that your original made at least two calls to those methods and possibly four on every iteration plus a call to isLeaf.  This version makes exactly three method calls, the same as the minimum in the original version.

Possible implementation:

    int getBiggerChildIndex(final int parentIndex) {
        int leftChildIndex = getLftChldIndx(parentIndex);
        int rightChildIndex = getRgtChildIndx(parentIndex);
        if (rightChildIndex <= end && heap[rightChildIndex] >= heap[leftChildIndex]) {
            return rightChildIndex;
        }

        return leftChildIndex;
    }

It's possible to write this with the ternary operator as well, but the condition is long enough that I think the if is worth it for readability.

This is also slightly more readable in that it doesn't guess which one is bigger so as to save an unnecessary variable assignment.  It returns instead.

Extraneous comment

            //if it is, then swap

Considering that this appears in an if statement and the next statement is swap, this is unnecessary.

Note that I actually removed all the comments.  I believe that my code structure makes them obvious enough to be unnecessary.  It's more borderline on the other two though.


Project Euler #5 (LCM of 1-20) Challenge: 2520 is the smallest number divisible by 1 to 10 without any remainder.
What is the smallest positive number that is evenly divisible by 1-20?

Solution:

public class Euler5 {
    public static void main(String[] args) {
        long startTime = System.nanoTime();
        int result = 0;

        for (int i = 2520; i > 0; i += 2520) {
            if (isSmallestMultiple(i)) {
                result = i;
                break;
            }
        }

        System.out.print("Result: " + result +
            ".\nTime used for calculation in nanoseconds: " +
            (System.nanoTime() - startTime) + ".");
    }

    public static boolean isSmallestMultiple(int i) {
        return i % 2 == 0 &&
            i % 3 == 0 &&
            i % 4 == 0 &&
            i % 5 == 0 &&
            i % 6 == 0 &&
            i % 7 == 0 &&
            i % 8 == 0 &&
            i % 9 == 0 &&
            i % 10 == 0 &&
            i % 11 == 0 &&
            i % 12 == 0 &&
            i % 13 == 0 &&
            i % 14 == 0 &&
            i % 15 == 0 &&
            i % 16 == 0 &&
            i % 17 == 0 &&
            i % 18 == 0 &&
            i % 19 == 0 &&
            i % 20 == 0;
    }
}

Example output:

 Result: 232792560.
Time Used for Calculation in nanoseconds: 11561136.

This is the first thing I tried. I started with a "should work" sentiment, and now that it has, I feel like it's an exceptional case.

Is incrementing by the given multiple, given its coverage of 1
through 10 more intuitive than I suspect? I'm wondering if this
situation is coincidental since they're not all primes.
Is there any obvious way of making this even faster?
Would this solution work for additional requirements? (i.e 1-25)

Your approach is.... accurate, but inefficient. I believe you already know that. You can search here on Code Review for alternate algorithms (I believe the most efficient algorithm is described in this answer by David Zhang (I see ILoveCoding has answered something similar).

But, I would like to address an obvious inefficiency here:

public static boolean isSmallestMultiple(int i) {
    return i % 2 == 0 &&
        i % 3 == 0 &&
        i % 4 == 0 &&
        i % 5 == 0 &&
        i % 6 == 0 &&
        i % 7 == 0 &&
        i % 8 == 0 &&
        i % 9 == 0 &&
        i % 10 == 0 &&
        i % 11 == 0 &&
        i % 12 == 0 &&
        i % 13 == 0 &&
        i % 14 == 0 &&
        i % 15 == 0 &&
        i % 16 == 0 &&
        i % 17 == 0 &&
        i % 18 == 0 &&
        i % 19 == 0 &&
        i % 20 == 0;
}

Right, you know that 4 is a multiple of 2.... so, why do you do:

`i % 2 == 0`

when you're also going to do:

`i % 4 == 0`

Similarly, why do 4 when you do 8, and 16?

If you do 16, then you've already also checked 8, 4, and 2.

At a minimum, if you're going to hard-code the values, at least hard-code the largest factors.... No need hard coding a value that has a larger multiple too in the tests:

public static boolean isSmallestMultiple(int i) {
    return
        i % 11 == 0 &&
        i % 12 == 0 &&
        i % 13 == 0 &&
        i % 14 == 0 &&
        i % 15 == 0 &&
        i % 16 == 0 &&
        i % 17 == 0 &&
        i % 18 == 0 &&
        i % 19 == 0 &&
        i % 20 == 0;
}


Slow Converting from JsonArray to ArrayList I have to convert a JsonArray to an ArrayList but unfortunately the code below is used very often during the execution of the program! How can I speed it up?

Java:

public static ArrayList CastJsonObject(JSONArray object) {
        ArrayList<Cantiere> tm = new ArrayList<Cantiere>();
        Cantiere cant;
        Cliente c;
        try {
            for (int i = 0; i < object.length(); i++) {
                JSONObject value = object.getJSONObject(i);
                int IdCantiere = Integer.parseInt(value.getString("IdCantiere").toString());
                int IdCliente = Integer.parseInt(value.getString("IdCliente").toString());
                String Filiale = value.getString("Filiale").toString();
                String RagioneSociale = value.getString("RagioneSociale").toString();
                String NomeCantiere = value.getString("NomeCantiere").toString();
                String DataCreazioneCantiere = value.getString("DataCreazioneCantiere").toString();
                String Tipologia = value.getString("Tipologia").toString();
                String StatoCantiere = value.getString("StatoCantiere").toString();
                int StatoFatturazione = Integer.parseInt(value.getString("StatoFatturazione").toString());
                c = new Cliente(IdCliente, RagioneSociale, Filiale);
                cant = new Cantiere(c, IdCantiere, NomeCantiere, DataCreazioneCantiere, Tipologia, StatoCantiere, StatoFatturazione);
                tm.add(cant);
            }
        } catch (Exception ex) {
            System.out.println("\n Error: " + ex);
        }

        return tm;
    }

If there really is a performance problem with that code, please back this up with some measurements. Otherwise, read some articles concerning premature optimization and don't bother.

Nevertheless, on micro-level there are a few calls that you don't need:

Get rid of superfluous .toString() calls, e.g. replace

String Filiale = value.getString("Filiale").toString();

with

String Filiale = value.getString("Filiale"); // this already is a string

Similarly, don't parse the integers yourself after converting them to string, but instead call getInt() on the Json object:

int IdCantiere = value.getInt("IdCantiere");

Avoid the repeated call to object.length() by holding the length in a variable:

for(int i = 0, n = object.length(); i < n; i++)

Still, the code should be relatively fast as-is. Naturally, there's no way to be really sure without knowing what the constructors for Cliente and Cantiere actually do. If you have a real performance problem, introduce measurements aroud the parameter extraction and the object construction.

And: measure BEFORE AND AFTER you change the code as shown above so that you actually know what effect it had. See https://stackoverflow.com/questions/504103/how-do-i-write-a-correct-micro-benchmark-in-java for some advice on how to do this.


Cut excess whitespace from input and print to output Title is pretty self explanatory, this program just takes in input (not from command line arguments), removes the extra spaces in the input, then prints that to output.

/*replace string of blanks with single blank*/

#include "stdio.h"

int main(int argc, char const *argv[])
{

    int nb,c = 0;

    while ((c=getchar()) != EOF){

           if (c == ' ' || c == '\n' || c == '\t')

                 if (nb >= 1)

                    nb = nb + 1;

                 else
                    nb = 1;
           else

                nb = 0;
        if (nb <= 1)
            putchar(c);

    }
    return 0;
}

Any suggestions on how to improve it?

A few suggestions:

Extract the space removal to a function of its own.  It a repeatable task and despite this being a short program it's still good practice. Then all you have to do in main() is just pass in the input string. Here's a function I came up with:

void removeSpaces(char* source)
{
    for (char *i = source, *j = source; *j != 0;)
    {
        *i = *j++;
        if(*i != ' ') i++;
    }
    *i = 0;
}

Keep in mind that this method modifies the original string.
Change how you get input from the user.  I'd recommend either getting input via command line arguments, or using fgets().  It's safer those ways and easier to do in my opinion.
If you decide to not use command line arguments, declare your function as:

int main(void)

Put the variable declarations to separate lines and initialize them to some value. From Code Complete, 2d Edition, p. 759:

  With statements on their own lines, the code reads from top to bottom,
  instead of top to bottom and left to right. When you’re looking for a
  specific line of code, your eye should be able to follow the left
  margin of the code. It shouldn’t have to dip into each and every line
  just because a single line might contain two statements.

You don't have to return 0 at the end of main(), just like you wouldn't bother putting return; at the end of a void-returning function. The C standard  knows how frequently this is used, and lets you not bother.

  C99 & C11 §5.1.2.2(3)

  ...reaching the } that terminates the main() function returns a
  value of 0.


Writing to and reading from socket in an IMAP client I implemented the following method to send IMAP commands to server and return the response to caller. Here, I'm not trying to parse the commands. Would like some advice on how I can improve the method (e.g. avoiding unnecessary copying) because I feel like I keep moving data between buffers and strings too much.

The other concern I have is about the size of the response. Right now, I'm returning a string. But this string can be potentially large (example fetch an email with a large email body or 100 email headers that contains subject, date and sender). In such a case, should I allocate response in the free-store and return a pointer to it or something like that? Thank you.

std::string cmail::ImapClient::execute(const std::string &command)
{
    std::stringstream ss;
    ss << ++commandCounter << " " << command;
    std::string cmd = ss.str();
    spdlog::trace("C: " + cmd);
    boost::system::error_code error;
    boost::asio::write(socket, boost::asio::buffer(cmd.append("\r\n")), error);
    if(error)
    {
        spdlog::error("Failed to dispatch command " + cmd + " because " + error.message());
        return "";
    }

    boost::asio::streambuf buffer;
    boost::asio::read_until(socket, buffer, '\n', error);
    if(error)
    {
        spdlog::error("Failed to receive response for command " + cmd + " because " + error.message());
        return "";
    }

    ss.str(std::string());
    ss.clear();
    std::istream is(&buffer);
    std::string line;
    while(std::getline(is, line))
    {
       ss << line << std::endl;
    }

    std::string response = ss.str();

    if(spdlog::default_logger()->level() == spdlog::level::trace)
    {
        std::string s = response;
        s.pop_back();
        spdlog::trace("S: " + s);
    }

    return response;
}

Thank you for your effort and time, helping me.

The code looks fine.  Instead of const std::string &command, you can take std::string_view command to avoid unnecessary allocation when the argument is not readily a std::string (e.g., string literals).  std::ostringstream seems more appropriate than std::stringstream here, since you are not reading from it.  Creating a new stream may also be more readable than reusing the stream.

Prior to C++20, str on string streams always make a copy of the buffer, so there isn't much you can do about it.  Since C++20, you can move the underlying buffer by moving the string stream itself:

// since C++20
std::string response = std::move(ss).str();


n largest files in a directory This is a script I wrote to find the n biggest files in a given directory (recursively):

import heapq
import os, os.path
import sys
import operator

def file_sizes(directory):
    for path, _, filenames in os.walk(directory):
        for name in filenames:
            full_path = os.path.join(path, name)
            yield full_path, os.path.getsize(full_path)

num_files, directory = sys.argv[1:]
num_files = int(num_files)

big_files = heapq.nlargest(
        num_files, file_sizes(directory), key=operator.itemgetter(1))
print(*("{}\t{:>}".format(*b) for b in big_files))

It can be run as, eg: bigfiles.py 5 ~.

Ignoring the complete lack of error handling, is there any obvious way to make this clearer, or at least more succinct? I am thinking about, eg, using namedtuples in file_sizes, but is there also any way to implement file_sizes in terms of a generator expression? (I'm thinking probably not without having two calls to os.path, but I'd love to be proven wrong :-)

You could replace your function with:

file_names = (os.path.join(path, name) for path, _, filenames in os.walk(directory)
        for name in filenames)

file_sizes = ((name, os.path.getsize(name)) for name in file_names)

However, I'm not sure that really helps the clarity.

I found doing this:

big_files = heapq.nlargest(
        num_files, file_names, key=os.path.getsize)
print(*("{}\t{:>}".format(b, os.path.getsize(b)) for b in big_files))

Actually runs slightly quicker then your version.


Platform game rendering method This copies the level graphics but uses a scale parameter. If the scale is 1, it copies a screen size segment. If it's 2, it copies a half-screen size piece and scales it to the screen size, etc.

How can I optimise this game level code or achieve the same outcome but simpler?

 public void render(Graphics g){

    int test = (screenwidth/2)-((screenwidth/2)/scale);
    int test2 = (screenheight/2)-((screenheight/2)/scale);

    int scaledstartx=test;
    int scaledstarty=test2;
    int scaledwidth=screenwidth-test;
    int scaledheight=screenheight-test2;
    int locationx=(wherex)+halfpwidth-(screenwidth/2);
    int locationy=(wherey)+halfpheight-(screenheight/2);

    g.drawImage(DrawnLevel,0,0,screenwidth,screenheight,
    scaledstartx+locationx,scaledstarty+locationy, scaledwidth+locationx,scaledheight+locationy, null );
 }

First, lets eliminate the test and test2 variables, which are uneeded (we already have scaledstartx and scaledstarty). Second keep screenwidth/2 and screenheight/2 in variables to avoid recalculating it over and over (the compiler will already do this, but doing manually the code will become clearer). Third, to increase the readability a bit, lets avoid doing calculations in the parameters.

So, this is your code now:

public void render(Graphics g) {

    int halfScreenWidth = screenwidth / 2;
    int halfScreenHeight = screenheight / 2;
    int scaledStartX = halfScreenWidth - (halfScreenWidth / scale);
    int scaledStartY = halfScreenHeight - (halfScreenHeight / scale);

    int scaledWidth = screenwidth - scaledStartX;
    int scaledHeight = screenheight - scaledStartY;
    int locationX = wherex + halfpwidth - halfScreenWidth;
    int locationY = wherey + halfpheight - halfScreenHeight;

    int spriteStartX = scaledStartX + locationX;
    int spriteStartY = scaledStartY + locationY;
    int spriteEndX = scaledWidth + locationX;
    int spriteEndY = scaledHeight + locationY;
    g.drawImage(DrawnLevel, 0, 0, screenwidth, screenheight, spriteStartX,
            spriteStartY, spriteEndX, spriteEndY, null);
 }

We can improve it by observing that the terms in the spriteblabla variables are decomposed in terms that have halfScreenWidth and halfScreenHeight both adding and subtracting. So, we can achieve a simpler expression. To do so, lets replace the variables in the assignment to the spriteblabla variables:

public void render(Graphics g) {

    int halfScreenWidth = screenwidth / 2;
    int halfScreenHeight = screenheight / 2;

    int spriteStartX = halfScreenWidth - (halfScreenWidth / scale) + wherex + halfpwidth - halfScreenWidth;
    int spriteStartY = halfScreenHeight - (halfScreenHeight / scale) + wherey + halfpheight - halfScreenHeight;
    int spriteEndX = screenwidth - (halfScreenWidth - (halfScreenWidth / scale)) + wherex + halfpwidth - halfScreenWidth;
    int spriteEndY = screenheight - (halfScreenHeight - (halfScreenHeight / scale)) + wherey + halfpheight - halfScreenHeight;

    g.drawImage(DrawnLevel, 0, 0, screenwidth, screenheight, spriteStartX,
            spriteStartY, spriteEndX, spriteEndY, null);
 }

Now, lets simplify that, since (halfScreenWidth - halfScreenWidth) and (screenwidth - halfScreenWidth - halfScreenWidth) are zero:

public void render(Graphics g) {

    int halfScreenWidth = screenwidth / 2;
    int halfScreenHeight = screenheight / 2;

    int spriteStartX = -(halfScreenWidth / scale) + wherex + halfpwidth;
    int spriteStartY = -(halfScreenHeight / scale) + wherey + halfpheight;
    int spriteEndX = (halfScreenWidth / scale) + wherex + halfpwidth;
    int spriteEndY = (halfScreenHeight / scale) + wherey + halfpheight;

    g.drawImage(DrawnLevel, 0, 0, screenwidth, screenheight, spriteStartX,
            spriteStartY, spriteEndX, spriteEndY, null);
 }

Now, moving some common subexpressions into variables:

public void render(Graphics g) {

    int halfScaledWidth = screenwidth / 2 / scale;
    int halfScaledHeight = screenheight / 2 / scale;
    int spriteCenterX = wherex + halfpwidth;
    int spriteCenterY = wherey + halfpheight;

    int spriteStartX = spriteCenterX - halfScaledWidth;
    int spriteStartY = spriteCenterY - halfScaledHeight;
    int spriteEndX = spriteCenterX + halfScaledWidth;
    int spriteEndY = spriteCenterY + halfScaledHeight;

    g.drawImage(DrawnLevel, 0, 0, screenwidth, screenheight, spriteStartX,
            spriteStartY, spriteEndX, spriteEndY, null);
 }


Word ladder implemented using stack and queue My function outputLadder function basically takes in a start and end string and generates the shortest word ladder from that.

It is iterating through a std::list containing over 3600 words read into it from a text file and seems to take a very long time.

What can I do to make my function faster while still maintaining a stack and queue implementation of the word ladder?

Here is the link to what the text file contains: http://textuploader.com/dkacm

Here is a video of the output which shows how long it takes.

Measuring it with clock() it takes about 8 seconds to find the shortest word ladder for style and crazy.

void WordLadder::outputLadder(const string &start, const string &end)
{
    stack<string> words;
    words.push(start);

    queue<stack<string>> ladder;
    ladder.push(words);

    while (!ladder.empty())
    {
        for (list<string>::iterator i = dictionary.begin(); i != dictionary.end(); ++i)
        {
            if (oneDiff(*i, ladder.front().top()))
            {
                if (*i == end)
                {
                    stack<string> output;

                    while (!ladder.front().empty())
                    {
                        output.push(ladder.front().top());
                        ladder.front().pop();
                    }

                    while (!output.empty())
                    {
                        cout << output.top() << " ";
                        output.pop();
                    }

                    cout << *i << endl;

                    return;
                }
                else
                {
                    stack<string> temp = ladder.front();
                    temp.push(*i);
                    ladder.push(temp);
                    i = dictionary.erase(i);
                }
            }
        }

        ladder.pop();
    }

    cout << "No word ladder exists for this word." << endl;
}

bool WordLadder::oneDiff(const string &dictWord, const string &stackWord)
{
    int count = 0;

    for (int i = 0; i < stackWord.size(); ++i)
    {
        if (dictWord.at(i) != stackWord.at(i))
        {
            ++count;
        }
    }

    if (count <= 1)
    {
        return true;
    }

    return false;
}

Some small improvements:

Do not use std::endl because it always forces a flush in addition to a new line, which can degrade output performance.
Prefer operator[] over at[] wherever possible. For example, in oneDiff you use dictWord.at(i) instead of dictWord[i]. The reason this is detrimental is that at() performs a bounds check (and possibly throws an exception if the bounds are violated) while operator[] does not. Since i only ever takes values smaller than stackWord.size(), you do not need at().
Prefer std::size_t for iteration variables. int may be to small to hold all indices for very large strings.
Don't use ... < something.somemethod() in a loop head, because something.somemethod() may be executed every iteration. If you know that it value does not change throughout, store its value before the loop into a variable.
oneDiff can be optimized. Since you only return whether or not the difference between characters is one or less, you can return false as soon as you have found more than one mismatch.
Although you do not give the definition of dictionary, it can be assumed that it is of type std::list<std::string> because of the iterator you are using. I would recommend replacing it with a std::vector if you are not doing a lot of insertion and deletion at random positions, because std::vector profits from cache coherency.