Pagination calculation I have written a function to get the actual page range to display that page range in pagination.

My function is working properly. But I think that I have written bad code. Can someone review my code and give me some feedback to improve my code?

Here is the code:

const pageRange = (() => {
  let startPage = activePage < pageNeighbours + 1 ? 1 : activePage - pageNeighbours;
  const endPage = totalPages < (pageNeighbours * 2) + startPage
    ? totalPages : (pageNeighbours * 2) + startPage;
  const diff = (startPage - endPage) + (pageNeighbours * 2);
  startPage -= (startPage - diff > 0 ? diff : 0);
  let actualPageRange = new Array((endPage - startPage) + 1)
    .fill().map((_, i) => i + startPage);
  if (actualPageRange[0] !== 1) actualPageRange = [1, ...actualPageRange];
  if (actualPageRange[1] !== 2) actualPageRange = [1, '...', ...actualPageRange.slice(1)];
  if (actualPageRange[actualPageRange.length - 1] !== totalPages) {
    actualPageRange = [...actualPageRange, totalPages];
  }
  if (actualPageRange[actualPageRange.length - 2] !== totalPages - 1) {
    actualPageRange = [...actualPageRange.slice(0, actualPageRange.length - 1), '...', totalPages];
  }
  return actualPageRange;
})();

In the above code:

activePage refers to the current page number.

pageNeighbours refers to the number of pages that should be shown besides active page

totalPages is self-explanatory

Update

After this function execution, I will use pageRange to create output like:

From a short review;

You use globals instead of parameters for activePage and pageNeighbours
You need at least one comment stating what pageNeighbours stands for, without knowing this the code looks meaningless to me
You need at least one comment on the top explaining what the range is supposed to contain
On the whole I think this could use more comments
Why actualPageRange instead of simply pageRange, or even range since it would be clear from context what range we're reading about

Okay, so since the code does not work, expect this question to be closed.
That doesn't mean that functionally this isn't a great question.

This is my approach to what I believe you are looking for:

const pageRange = ((activePage, pageNeighbours, totalPages) => {

  let range = [];
  //Add active page and neighbouring pages
  for(let page = activePage - pageNeighbours; page <= activePage + pageNeighbours; page++)
    range.push(page);
  //Make sure we dont show pages that dont exist
  range = range.filter(page=> page > 0 && page <= totalPages);
  //Allow user to go to the first page if need be
  //The second entry should be either 2 or ellipsis(...)
  if(range[0]!=1){
    if(range[1]!=2){
      range.unshift('...');
    }
    range.unshift(1);
  }
  //Allow user to go the last page, second last entry should be second last page or ellipsis
  if(range[range.length-1] != totalPages){
    if(range[range.length-2]!= totalPages-1){
      range.push('...');
    }
    range.push(totalPages);
  }

  return range;
})(activePage, pageNeighbours, totalPages);

Note that this avoids accessing activePage etc. as a global.


Multiple input fields from PHP table To summarize from a previous question, the user selected a starting date, an ending date, and a region. From these choices, an array of weeks and regions are created, and users enter values ("points") that correspond to a particular week and region. This is what it looks like right now (some parts are hard-coded for testing purposes):

$date_array = Array("01/01/2012", "01/08/2012", "01/15/2012");
$region_array = Array("NYC", "DC");

<form name="test_form" id="test_form" method="post">

        <?php
            foreach ($date_array as $date)
                {
                echo "".$date."	";
                }
        ?>

    <?php
    foreach ($region_array as $region)
        {
        echo "
";
        echo "".$region."	";
        foreach ($date_array as $date)
            {
            echo '<input type="text" name='.$region."-value1_".$date.'><input type="text" name='.$region."-value2_".$date.'>	';

            }
        echo "
";
        }
    ?>

<input type="submit" name="submit">

And the next part:

if (isset($_POST['submit']))
{
    foreach ($_POST as $k=>$v)
    {
    if ($k != "submit") // use even and odd count to differentiate TRP and CPP
        {
        // split the form input on _
        $input = explode("_", $k);
        echo "
Region: " . $input[0];
        echo "
Date: " . $input[1];
        echo "
Value: " . $v;
        }
    echo "
";
    }
}

So basically, in this example it outputs 12 times:

Region: NYC-value1
Date: 01/01/2012
Value: 500

Region: NYC-value2
Date: 01/01/2012
Value: 5

Region: NYC-value1
Date: 01/08/2012
Value: 600

I want to catch the value1 and value2 while the loop is running, but as it stands it loops through both of them before going onto the next date/region. My idea, as I put in comments, was to have a counter of even and odd numbers and simply tell if it was doing value1 or value2 based on that.

Is this a good idea or am I missing something fundamentally simpler?

Try this:

<?php
if(isset($_POST['submit'])){
    foreach ($_POST as $k=>$v){
        // use even and odd count to differentiate TRP and CPP
        if ($k != "submit") {
            // split the form input on _
            $input = explode("_", $k);
            echo "
Region: " . $input[0];
            echo "
Date: " . $input[2];
            if($input[1] == "value1"){
                echo "
Value 1: " . $v;
            }
            elseif($input[1] == "value2"){
                 echo "
Value 2: " . $v;
            }
         }
        echo "
";
     }
}
else {
    $date_array = Array("01/01/2012", "01/08/2012", "01/15/2012");
    $region_array = Array("NYC", "DC");
?>
    <form name="test_form" id="test_form" method="post" action="">

<?php
                foreach ($date_array as $date){
                    echo "".$date."	";
                }
?>

<?php
        foreach ($region_array as $region){
            echo "
";
            echo "".$region."	";
            foreach ($date_array as $date){
                echo '<input type="text" name='.$region."_value1_".$date.'><input type="text" name='.$region."_value2_".$date.'>	';
            }
            echo "
";
        }
?>

    <input type="submit" name="submit">
</form>
<?php
}
?>


jQuery hover and click events for a five-star rating This seems a little bit excessive, and was wondering if I can refactor this jQuery. I'm not sure if you need anymore information, or if it's possible just by looking at this script.

  var getActiveState;
  $(".modal-review__rating-order-wrap span").hover(function(){
    $(this).parent().removeClass("is-active");
    var thisStar = $(this).parent().find("[data-rating-value="+getActiveState+"]")
    thisStar.removeClass("is-active");
  }, function(){
    var thisStar = $(this).parent().find("[data-rating-value="+getActiveState+"]")
    if(getActiveState){
      $(this).parent().addClass("is-active");
      thisStar.addClass("is-active");
    }
  });
  $(".modal-review__rating-order-wrap span").on("click",function(){
    $(this).parent().addClass("is-active");
    $(".modal-review__rating-order-wrap span").not(this).removeClass("is-active");
    $(this).addClass("is-active");
    getActiveState = $(this).data("rating-value");
    return getActiveState;
  });

This is applied to a 5 star system, wanted to show what the jQuery is affecting. When jQuery clicks on the star, it adds an .is-active class to the span, and also to the wrap.

    <svg>star 1</svg>

    <svg>star 2</svg>

    <svg>star 3</svg>

    <svg>star 4</svg>

    <svg>star 5</svg>

I couldn't see the Unicode stars in 200_success's answer on Chrome, so I made a version of their snippet with SVG stars from an old meta.SO post of mine:

$('.modal-review__rating-order-wrap > span').click(function() {
    $(this).addClass('active').siblings().removeClass('active');
    $(this).parent().attr('data-rating-value', $(this).data('rating-value'));
});
/* Initial state */
div.modal-review__rating-order-wrap > span {
    display: block; float: left;
    height: 30px; width: 40px;
    background-image: url("data:image/svg+xml,%3Csvg%20xmlns='http://www.w3.org/2000/svg'%20width='80'%20height='30'%3E%3Cpath%20d='M17.5,12.5h-8.5l6.8,5-2.6,8.1,6.8-5,6.8,5-2.6-8.1,6.8-5h-8.5l-2.6-8.1z'%20fill='%23c0c0c0'%20stroke='%23c0c0c0'/%3E%3Cpath%20d='M57.5,12.5h-8.5l6.8,5-2.6,8.1,6.8-5,6.8,5-2.6-8.1,6.8-5h-8.5l-2.6-8.1z'%20fill='%23ffd83d'%20stroke='%23eac328'/%3E%3C/svg%3E");
    background-position: 0px 0px;    /* gray star */
}

/* Persistent state */
div.modal-review__rating-order-wrap[data-rating-value] > span {
    background-position: -40px 0px;  /* gold star */
}
div.modal-review__rating-order-wrap > span.active ~ span {
    background-position: 0px 0px;    /* gray star */
}

/* Hover state */
div.modal-review__rating-order-wrap[class]:hover > span {
    background-position: -40px 0px;  /* gold star */
}
div.modal-review__rating-order-wrap[class] > span:hover ~ span {
    background-position: 0px 0px;    /* gray star */
}

Note that I had to add [class] as a specificity hack to the last two CSS selectors, since otherwise the third selector (with .active) would override them.  (In 200_success's version, the :after pseudo-classes effectively serve the same function, but my version doesn't use them.)

I also made the inner spans into floating blocks to eliminate any whitespace gaps between them, since those cause annoying "blinking" effects when moving the mouse over the gap.  Flex layout on the outer div could be used to achieve the same effect.

Note that I've marked this "answer" as Community Wiki, since it's basically just an addendum to another answer.  I'd have posted it as a comment, but you can't include snippets in comments.  I didn't feel presumptuous enough to edit this into their answer directly, but if 200_success wants to do so, I'm happy to delete this "meta-answer".


Merge sort could work 10 times faster I am implementing merge sort in Java and I have a performance problem in this method:

    private static void marge(int[] source, int[] buffer, int startingIndex, int count) {
        int index1 = startingIndex;
        int index2 = startingIndex + count / 2;
        int maxIndex1 = index2;
        int maxIndex2 = startingIndex + count;

        for(int i = 0; i < count; i++){
            if(index2 >= maxIndex2 || (index1 < maxIndex1 && source[index1] < source[index2])){
                buffer[startingIndex + i] = source[index1];
                index1++;
            }
            else{
                buffer[startingIndex + i] = source[index2];
                index2++;
            }
        }

//      System.arraycopy(source, startingIndex, buffer, startingIndex, count);
    }

If I replace this method content with the content of the comment, like this:

private static void marge(int[] source, int[] buffer, int startingIndex, int count) {
    System.arraycopy(source, startingIndex, buffer, startingIndex, count);
}

It performs 10 to 20 times faster. I understand that I have more calculations in my original method but "10 to 20 times" seems too much for me.

Do you have any idea how to improve performance?

Some pointers:

You keep adding 2 numbers here: startingIndex + i, that could probably be avoided if you went for(int i = startIndex; i < lastIndex; i++){ where lastIndex would be startIndex + count
Since ++ happens post evaluation you could simply do buffer[startingIndex + i] = source[index1++];, it does not have to be a separate statement
Short circuit logic : if index2 >= maxIndex2, then you should probably have a  System.arraycopy copy that fills the rest into index1 and get out. You could the same for when index1 > maxIndex1 but obviously you would fill into index2 then

From a naming perspective, I would rather read upperIndex and lowerIndex than index1 and index2

Totally untested, I would try something like this:

private static void merge(int[] source, int[] buffer, int startingIndex, int count) {
    int lowerIndex = startingIndex;
    int lowerBound = startingIndex + count / 2;
    int upperIndex = lowerBound;
    int upperBound = startingIndex + count;

    for(int i = startingIndex ; i < upperBound ; i++){
        if(upperIndex >= upperBound ){
          System.arraycopy(source, lowerIndex , buffer, i, i - startingIndex );
          return;
        }
        if(lowerIndex >= lowerBound ){
          System.arraycopy(source, upperIndex , buffer, i, i - startingIndex );
          return;
        }
        buffer[i] = source[lowerIndex] < source[upperIndex] ? source[lowerIndex++] : source[upperIndex++]
    }
}


Getting validation from inherited classes I'm building some code with an inheritance heirarchy, and I want to validate the resulting instances. At present I have a method in the base class can override to return a list of the problems (see the code).

My problem here is that this works, provided the inheriting classes remember to get the base results first, before returning their own. It works, but I'm not sure it's the best design?

using System;
using System.Linq;
using System.Collections.Generic;

namespace Query
{
    /// <summary>
    /// Root class with mehtod
    /// </summary>
    public abstract class baseClass
    {

        /// <summary>
        /// Is the object valid?
        /// </summary>
        /// <returns></returns>
        public bool IsValid()
        {
            return (!GetValidationIssues().Any());
        }

        /// <summary>
        /// Return list of validation issues
        /// </summary>
        /// <returns></returns>
        public virtual List<string> GetValidationIssues()
        {
            var result = new List<string>();
            if (Quantity < 0) result.Add("Quantity is invalid");
            return result;
        }

        public int Quantity { get; set; }
    }

    /// <summary>
    /// First child class, adds a new property and more validation
    /// </summary>
    public abstract class productBase : baseClass
    {
        public override List<string> GetValidationIssues()
        {
            // we must get any base results first!!
            var result = base.GetValidationIssues();
            // do checks for this class
            if (string.IsNullOrWhiteSpace(Name)) result.Add("Name not specified");
            return result;
        }

        public string Name { get; set; }
    }

    /// <summary>
    /// Final instance
    /// </summary>
    public class Product : productBase
    {
        public override List<string> GetValidationIssues()
        {
            // remember to get the base results first
            var result = base.GetValidationIssues();
            // do checks for this class
            if (Price <= 0) result.Add("Price is not valid");
            return result;
        }

        public decimal Price { get; set; }
    }

}

Here's a C# version using a cross of the template method pattern and the collector pattern.
It'll pick up any issues from any derivative implementing the AddValidationIssues method.

public abstract class BaseClass
{
    public List<string> GetValidationIssues()
    {
        var result = new List<String>();
        if (Quantity < 0) result.Add("Quantity is invalid");
        AddValidationIssues(result);
        return result;
    }

    protected virtual void AddValidationIssues(List<string> result)
    {
    }
}

public class ProductBase : BaseClass
{
    protected override void AddValidationIssues(List<string> result)
    {
        if (string.IsNullOrWhiteSpace(Name)) result.Add("Name not specified");
    }
}

As a sidenote, I'd look for another name than "BaseClass". It doesn't say anything about what it does.


Simple file downloader On my website I want to offer some PDFs, ZIP files and images for download. They are all in the same folder and I could simply link to them like download file. But instead I want to invoke a download dialog by sending an attachment header via php. The link would change into download file:

<?php
/***
 * download/load.php
 * Simple file downloader including mime type
 *
 * @param string $f  name of file in download folder
 * @return file with corresponding HTTP header
 */

// check whether parameter was sent
if ( !isset($_GET['f']) ||
// check whether it is not `load.php` (this file)
     $_GET['f'] === "load.php" ||
// check whether file exists
     !is_readable($_GET['f']) ||
// make sure file is in the current directory
     strpos($_GET['f'], '/') ) {
    header("HTTP/1.0 404 Not Found");
    die();
}

$file = $_GET['f'];

// check mime type
$finfo = finfo_open(FILEINFO_MIME_TYPE);
$mime = finfo_file($finfo, $file);
finfo_close($finfo);

// send mime type
header('Content-type: ' . $mime);

// invoke download dialog
header('Content-Disposition: attachment; filename=' . $file);

// send file
readfile($file);
?>

Is this script okay or does it have security vulnerabilities? If so, how could I improve it?

Take care to call strpos() correctly. If the f parameter contains a leading /, it sneaks through.


Rolling Average improvements I have created a class intended to average the previous few data points submitted. It is able to change in size and has a feature to bypass the average without affecting the data contained therein. Are there any improvements I could make, or any exceptions thrown in certain situations that I haven't thought of?

public class RollingAverage {

    private double[] data;
    private boolean bypass = false;

    public RollingAverage(int size) {
        if (size == 1) {
            bypass = true;
        }

        data = new double[size];
        Arrays.fill(data, Integer.MIN_VALUE);
    }

    /**
     * Adds a data point to the rolling average
     *
     * @param newData
     * @return the new average
     */
    public double addData(double newData) {
        if (bypass) {
            return newData;
        }
        boolean full = true;
        for (double point : data) {
            if (point == Integer.MIN_VALUE) {
                full = false;
                break;
            }
        }

        if (full) {
            int j = 0;
            while (j < data.length - 1) {
                data[j++] = data[j];
            }
            data[0] = newData;

            int sum = 0;
            for (double point : data) {
                sum += point;
            }
            return sum / data.length;
        } else {
            double sum = newData;
            int j = 0;
            while (data[j] != Integer.MIN_VALUE) {
                sum += data[j++];
            }
            data[j] = newData;
            return sum / (j + 1);
        }
    }

    public void changeSize(int size) {
        if (size == data.length) {
            return;
        }

        if (size == 1) {
            bypass = true;
        }

        double[] oldData = data;
        data = new double[size];

        for (int i = 0; i < size; i++) {
            if (i < oldData.length) {
                data[i] = oldData[i];
            } else {
                data[i] = Integer.MIN_VALUE;
            }
        }
    }

    public void setBypass(boolean bypass) {
        this.bypass = bypass;
    }

    public void clear() {
        Arrays.fill(data, Integer.MIN_VALUE);
    }

    public int getAverage() {
        return (int) getAverageDouble();
    }

    public double getAverageDouble() {
        int sum = 0;
        int count = 0;
        for (double point : data) {
            if (point != Integer.MIN_VALUE) {
                sum += point;
                count++;
            } else {
                break;
            }
        }
        return sum / count;
    }
}

On new RollingAverage(-1) it throws a NegativeArraySizeException. An IllegalArgumentException with a proper message would be better here since clients should not know that you are using arrays in the implementation.
On new RollingAverageImpl(1).getAverage() it throws an ArithmeticExpression. What about an IllegalStateException here?
If you are using Integer.MIN_VALUE as a special marker value the addData method should not accept it. Just throw an IllegalArgumentException. (I'd consider using Double.NaN here as a named constant, like EMPTY_DATA or just EMPTY.)
Comparing doubles with integers (like point == Integer.MIN_VALUE) might not be precise. See: Why not use Double or Float to represent currency?
If you calculate the new average after every addData call maybe you should cache it and use the cached value in the  getAverageDouble method.
Actually, I'd cache the sum too, and in the addData subtract the old value from the sum (if the array is full) and add the newData to it, then divide it by the number of elements. Maybe there are better algorithms.


Using T type parameter to clone a collection I want to clone a collection with the following method and I want to know if it can be optimized:

public T CloneCollection<T>(IEmployees employees) where T : IEmployees, new()
{
    T collection = new T();
    foreach (IEmployee employee in employees)
    {
        collection.Add(employee);
    }
    return collection;
}

These are the other classes which are related to the method above:

public class RegularEmployee : Entities, IEmployee
{
//properties
}

public class User : Entities, IEmployee
{
//properties
}

public class Manager : Entities, IEmployee
{
//properties
}

public class ManagerCollection : List<Manager>, IEmployees
{
   void Add(IEmployee employee);
}

public class RegularEmployeeCollection : List<RegularEmployee>, IEmployees
{
   void Add(IEmployee employee);
}

public class UserCollection : List<User>, IEmployees
{
   void Add(IEmployee employee);
}

You can replace your loop with AddRange():

public T CloneCollection<T>(IEmployees employees) where T : IEmployees, new()
{
    T collection = new T();
    collection.AddRange(employees);
}

Additional thought:

Note that your solution and this solution are simply cloning the list and not the items in the list.  If you update an Employee, the employee will be modified in both lists.  If this is NOT the desired behavior, you will need to clone each employee as well.


Find Row with Max zero To find the row  with max zero Matrix will have either 0 or 1 in sorted manner.

 public static void main(String[] args) {
    // TODO Auto-generated method stub
    int[][] inp=new int[][]{
                            {0,0,0,1,1},
                            {0,1,1,1,1},
                            {0,0,1,1,1},
                            {0,0,0,0,1}
                            };
    int j=0,rownum=0;
    for(int i=0;i<4;i++){
        while(j<=4&&inp[i][j]!=1){
            rownum=i;
            j++;
        }
    }
    System.out.println("Row "+(rownum+1)+" has Max zero");

}

Complexity of the  above program is \$O(m+n)\$ where \$m\$ is row size and \$n\$ is column size.

If any better approach is there then please post in this

I do not think you can do something to reduce Complexity in any traditional way. You have to traverse all array elements in order to find the row with the max element,so m x n iterations will give you O(m+n) Complexity.


Find two numbers that equal to target value Background
Given some random array of numbers like [3, 5, -4, 8, 11, 1, -1, 6] and a target value 10, find two numbers within the array that equal to the target value.
Code
def two_Number_Sum(array, target):
    unique_number_set = set()
    for num in array:
        required_value = target - num
        if required_value in unique_number_set:
            return [num, required_value]
        else:
            unique_number_set.add(num)
    return []

print(two_Number_Sum([3, 5, -4, 8, 11, 1, -1, 6], 10))

I am wondering if I can make this answer more pythonic. I don't have a Python background so a lot of times I write code like Java or another language, and I figure it will be good practice to critique myself and write code that is more pythonic so it becomes natural.

Few suggestions:

Naming: array is not an array, a more appropriate name can be numbers. Also unique_number_set can be just unique_numbers, the name already tells that there are no duplicates.
Unnecessary else: the else is not required so it can be removed.
Type hints: @Linny already pointed this out. I add that, if the result doesn't need to be modified, consider returning a tuple. Additionally, it tells the caller that the result will only contain two numbers.

from typing import List, Tuple, Union

def two_number_sum(numbers: List[int], target: int) -> Union[Tuple[int, int], Tuple[()]]:
    unique_numbers = set()
    for num in numbers:
        required_value = target - num
        if required_value in unique_numbers:
            return num, required_value
        unique_numbers.add(num)
    return ()

Thanks to @KellyBundy and @AJNeufeld for the feedbacks in the comments.


LeetCode: Sort The Matrix Diagonally C# https://leetcode.com/problems/sort-the-matrix-diagonally/

A matrix diagonal is a diagonal line of cells starting from some cell
in either the topmost row or leftmost column and going in the
bottom-right direction until reaching the matrix's end. For example,
the matrix diagonal starting from mat[2][0], where mat is a 6 x 3
matrix, includes cells mat[2][0], mat3, and mat[4][2].
Given an m x n matrix mat of integers, sort each matrix diagonal in
ascending order and return the resulting matrix.

my idea is to run as a BFS on the matrix. each cell visit top and right cells. to form a diagonal and sort it.
please review for performance. it took 30 mins to write the code. review this please as this is a coding interview.
using Microsoft.VisualStudio.TestTools.UnitTesting;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;

namespace ArrayQuestions
{
    /// <summary>
    /// https://leetcode.com/problems/sort-the-matrix-diagonally/
    /// </summary>
    [TestClass]
    public class MatrixDiagonalSortTest
    {
        [TestMethod]
        public void TestMethod1()
        {
            int[][] input = { new[] { 3, 3, 1, 1 }, new[] { 2, 2, 1, 2 }, new[] { 1, 1, 1, 2 } };
            int[][] output = { new[] { 1, 1, 1, 1 }, new[] { 1, 2, 2, 2 }, new[] { 1, 2, 3, 3 } };
            var res = DiagonalSort(input);
            for (int r = 0; r < res.Length; r++)
            {
                CollectionAssert.AreEqual(output[r], res[r]);
            }

        }
        public int[][] DiagonalSort(int[][] mat)
        {
            Queue<Cell> Q = new Queue<Cell>();
            Q.Enqueue(new Cell(mat.Length-1, 0, mat[mat.Length-1][0]));
            while (Q.Count > 0)
            {
                List<Cell> tempList = new List<Cell>();
                int count = Q.Count;
                for (int i = 0; i < count; i++)
                {
                    var curr = Q.Dequeue();
                    tempList.Add(curr);
                    int newR = curr.r - 1;
                    int newC = curr.c;
                    if (newR >= 0)//go up
                    {
                        if (Q.FirstOrDefault(x => x.r == newR && x.c == newC) == null)
                        {
                            Q.Enqueue(new Cell(newR, newC, mat[newR][newC]));
                        }

                    }
                    newR = curr.r;
                    newC = curr.c + 1;
                    if (newC <= mat[0].Length - 1)//go right
                    {
                        if (Q.FirstOrDefault(x => x.r == newR && x.c == newC) == null)
                        {
                            Q.Enqueue(new Cell(newR, newC, mat[newR][newC]));
                        }
                    }
                }
                var listValues = new List<int>();
                foreach (var item in tempList)
                {
                    listValues.Add(item.v);
                }
                listValues.Sort();
                for (int i = 0; i < tempList.Count; i++)
                {
                    mat[tempList[i].r][tempList[i].c] = listValues[i];
                }
            }
            return mat;
        }
    }
    [DebuggerDisplay("r ={r}, c={c}, v={v}")]
    public class Cell
    {
        public Cell(int r1, int c1, int v1)
        {
            r = r1;
            c = c1;
            v = v1;

        }
        public int r { get; set; }
        public int c { get; set; }
        public int v { get; set; }
    }
}

Few tips

For code quality, consider to give variables/fields more understandeable names.
For immutable Cell fields add readonly, it may affect the performance.
If you know how many items will be put in the collection, construct it with capacity new List<Cell>(count), capacity means initial size for the underlying storage not max items limit.
FirstOrDefault == null can be replaced with !Any.
You may add items directly to listValues i guess, and optimize out tempList or vice-versa. Then Sort by field with Linq, or custom comparer, or implement IComparable for Cell.
Cell can be struct? If yes, you can try to take an advantage of stack allocations (for small amount of data) instead of heap using Span<T>-related optimizations.

For 5th tip, this code (as far as I got the idea)
var listValues = new List<int>();
foreach (var item in tempList)
{
    listValues.Add(item.v);
}
listValues.Sort();
for (int i = 0; i < tempList.Count; i++)
{
    mat[tempList[i].r][tempList[i].c] = listValues[i];
}

Can be optimized as
int i = 0;
foreach (var item in tempList.OrderBy(x => x.v))
{
    mat[tempList[i].r][tempList[i].c] = item.v;
    i++;
}

Btw, I've tested various optimizations from listed above and more but got only insignificant performance boost. The slowest point is here FirstOrDefault(x => x.r == newR && x.c == newC). I have no idea how to improve it without moving the solution to different approach.
Ok, let's do it.
public int[][] DiagonalSort(int[][] mat)
{
    int width = mat[0].Length;
    int height = mat.Length;
    List<int> values = new List<int>(height);
    for (int j = 2 - height; j < width - 1; j++)
    {
        for (int i = 0; i < height; i++)
        {
            int offset = j + i;
            if (offset >= 0 && offset < width)
                values.Add(mat[i][offset]);
        }
        values.Sort();
        for (int i = 0, count = 0; i < height; i++)
        {
            int offset = j + i;
            if (offset >= 0 && offset < width)
                mat[i][offset] = values[count++];
        }
        values.Clear();
    }
    return mat;
}

~15% faster than initial solution.


Maximize the number of PyGame sprites, with collision detection I was trying to see what is the highest number of moving and colliding boxes (sprites) I could get in pygame?  So that I could start working on game I want,  I searched google and I found different people do it as a challenge, trying to get to the highest number of sprites. But none of them use the colliding, they just make a high number of sprites with no for loop inside the main loop.

The highest number I could get before it started to lag was just 260, which is too low for the game I want to start making.

Here is my code:

import pygame
from pygame.locals import*
import time
screen=pygame.display.set_mode((1250,720))
class Box(pygame.sprite.DirtySprite):
    def __init__(self,posx,posy):
        pygame.sprite.DirtySprite.__init__(self)
        self.image= pygame.Surface ([40,40]).convert_alpha()
        self.image.fill((250,0,0))
        self.rect=self.image.get_rect()
        self.rect.center=(posx,posy)
    def update (self):
        self.dirty=1
pygame.init()
clock=pygame.time.Clock()
boxs=pygame.sprite.LayeredDirty()
while True :
    start = time.time()
    screen.fill((0,0,0))
    for event in pygame.event.get():
        if event.type==pygame.QUIT :
            pygame.quit()
            quit()
        if event.type== pygame.MOUSEBUTTONDOWN and event.button == 1:
            box=Box(event.pos[0],event.pos[1])
            inside=pygame.sprite.spritecollideany(box,boxs )
            if not inside :
                boxs.add(box)
        elif event.type== pygame.MOUSEBUTTONDOWN and event.button == 3:
            print (boxs)
        elif event.type==KEYDOWN:
            if event.key==K_SPACE:
                boxs.empty()
    for box in boxs :
         if box.rect.y<650 :
##             touch= pygame.sprite.spritecollideany(box, boxs)
##             if touch==box:
##this way above didnt work for me because the box will cross any other box added to the group after it
##for example the box number 3 in the boxs will cross box number 4,5,6,7,8,9,........
             touch= pygame.sprite.spritecollide(box, boxs,False)
             if (len(touch))<2:# i do this because (touch) will always return that the box touch it self
                               # but when its touch more then one then i know it touch other box so it will not move
                               # and i feel that there is much faster way to do this check then my way :(
                 box.rect.move_ip(0,1)
    boxs.update()
    dirty=boxs.draw(screen)
    pygame.display.update(dirty)
    clock.tick(60)
    end = time.time()
    print (end-start)
#the best until time totally turn to 0.02xxxx = 260 sprite

So what else I could do to have more then 260 moving and colliding boxes at 60 fps without lag?

A quick suggestion. If you look at the implementation of spritecollide, you'll see that it iterates over all the sprites in the group to see if the rectangles overlap. This means that the loop:

for box in boxs:
    # ...
    touch = pygame.sprite.spritecollide(box, boxs, False)

takes time proportional to the square of the number of boxes.

What I would try instead would be to insert the boxes into a spatial index which can find intersections efficiently. For example, put them in an R-tree using the Rtree package, and then call the intersection method. Or put their centres in a kd-tree using scipy.spatial.KDTree and then call the query_pairs method, passing the size of the largest box as the search radius.

Responses to comments

There are ways to update spatial indexes as objects move (typically in games objects don't move very far in a frame, so updates tend to be local). But the simplest thing to do is to rebuild the spatial index each frame — this has cost \$\Theta(n\log n)\$ which is an improvement on the \$\Theta(n^2)\$ approach of comparing all pairs of objects.
Here's an example using scipy.spatial.KDTree. Using this data structure requires three steps. First, take your list of sprites and build a tree containing their centres:

from scipy.spatial import KDTree
tree = KDTree([sprite.rect.center for sprite in sprites])

Second, query the tree for candidate collisions, using an appropriate search distance (in this case the diameter of the largest sprite):

from math import hypot
distance = max(hypot(*sprite.rect.size) for sprite in sprites)
collisions = tree.query_pairs(distance)

Third, not all of the candidates are actually colliding (because we have only queried the distance between their centres), so check to see which ones are real:

for i, j in collisions:
    if sprite[i].rect.colliderect(sprite[j].rect):
        # handle collision between sprite[i] and sprite[j]


Small program to get binary diff offset between two small files Trivial utility program meant to go through the list of (small) files and report file offset of the first difference between them. The goal of the code is simplicity.

Any recommendations are useful. Be brutal. I'm collecting go experience :)

package main

import (
    "fmt"
    "io/ioutil"
    "os"
)

func main() {
    if len(os.Args) < 3 {
        fmt.Println("expecting at least 2 arguments")
        return
    }

    nfi := len(os.Args) - 1

    bufs := make([][]byte, nfi)

    // read in the files
    for i, fn := range os.Args[1:] {
        fi, err := os.Open(fn)
        if err != nil {
            fmt.Printf("error opening file %s: %v", fn, err)
            return
        }
        defer fi.Close()

        bufs[i], err = ioutil.ReadAll(fi)
        if err != nil {
            fmt.Printf("error reading from file %s: %v", fn, err)
        }
    }

    // get a minimum len of all the buffers
    min := len(bufs[0])
    for _, b := range bufs[1:] {
        l := len(b)
        if l < min {
            min = l
        }
    }

    // compare values one by one
    for i := 0; i < min; i++ { // loop over offset

        v := bufs[0][i]            // a byte from first file
        for j := 1; j < nfi; j++ { // loop over the rest of files
            if v != bufs[j][i] {
                fmt.Printf("first difference at offset %d = 0x%x\n", i, i)
                return
            }
        }
    }

    fmt.Println("no differences found")
}

To understand your program better, I cleaned up up your code.

diff.go:

package main

import (
    "fmt"
    "io/ioutil"
    "os"
)

func main() {
    files := os.Args[1:]
    if len(files) < 2 {
        fmt.Fprintln(os.Stderr, "expecting at least 2 files")
        return
    }

    bufs := make([][]byte, len(files))
    for i, file := range files {
        var err error
        bufs[i], err = ioutil.ReadFile(file)
        if err != nil {
            fmt.Fprintln(os.Stderr, "error reading file %s: %v", file, err)
            return
        }
    }

    minLen := len(bufs[0])
    for _, b := range bufs[1:] {
        if len(b) < minLen {
            minLen = len(b)
        }
    }

    for i := 0; i < minLen; i++ {
        v := bufs[0][i]
        for j := 1; j < len(files); j++ {
            if v != bufs[j][i] {
                fmt.Printf("first difference at offset %[1]d = %#[1]x\n", i)
                return
            }
        }
    }

    fmt.Println("no differences found")
}

Code should be readable. Drop references to os.Args; It's files.

For example, unreadable code and messages:

if len(os.Args) < 3 {
    fmt.Println("expecting at least 2 arguments")
    return
}

and so on.

Readable code and messages:

files := os.Args[1:]
if len(files) < 2 {
    fmt.Fprintln(os.Stderr, "expecting at least 2 files")
    return
}

and so on.

Also, error messages go to stderr.

In Go, lengths are pre-calculated. Delete

nfi := len(os.Args) - 1

nfi is simply the efficient and meaningful len(files).

Why is it so complicated to read file data?

    fi, err := os.Open(fn)
    if err != nil {
        fmt.Printf("error opening file %s: %v", fn, err)
        return
    }
    defer fi.Close()

    bufs[i], err = ioutil.ReadAll(fi)
    if err != nil {
        fmt.Printf("error reading from file %s: %v", fn, err)
    }

Note: defer fi.Close() doesn't work the way you want it to. The defers don't run until main returns.

Simplify, just ReadFile,

    var err error
    bufs[i], err = ioutil.ReadFile(file)
    if err != nil {
        fmt.Fprintln(os.Stderr, "error reading file %s: %v", file, err)
        return
    }

You wrote

    if l < min {
        min = l
    }

It's more readable to line them up

    if min > l {
        min = l
    }

I found the variable name min a little terse. I used minLen. And, again, l := len(b) is redundant. len(b) is pre-calculated.

// get a minimum len of all the buffers
min := len(bufs[0])
for _, b := range bufs[1:] {
    l := len(b)
    if l < min {
        min = l
    }
}

became

minLen := len(bufs[0])
for _, b := range bufs[1:] {
    if minLen > len(b) {
        minLen = len(b)
    }
}

You write:

fmt.Printf("first difference at offset %d = 0x%x\n", i, i)

Why 0x%x?

Why  i, i?

Using the fmt package documentation:

fmt.Printf("first difference at offset %[1]d = %#[1]x\n", i)


Review

Yes the first option would be better as memory might become an issue with really long numbers (say you have a 10 million length number). But that implementation could be improved as well

Your first solution does not return the max product but rather a list generator of products. This is confusing!
Chop up your code into functions, as it makes it easier to test/change things

The problem can be divided into to 3 seperate functions:

Cut a big number into evenly sized chunks of adjacent digits
Sum the product of the chunks
A main function that returns the largest adjecent product

Alternative Code

from functools import reduce
import operator

def cut_adj_chunks(iterable, chunk_size):
    for i in range(len(iterable) - chunk_size):
        yield iterable[i:i+chunk_size]

def sum_prod(num_str):
    num_list = map(int, num_str)
    return reduce(operator.mul, num_list, 1)

def largest_prod(adj_digits, big_num):
    return max(sum_prod(chunk) for chunk in cut_adj_chunks(str(big_num), adj_digits))


A simple implementation of the producer/consumer pattern Consider this class that consumes some items and handles them with the provided handler function:

#include <queue>
#include <mutex>
#include <atomic>
#include <thread>
#include <utility>
#include <condition_variable>

template < typename Item >
struct Handler
{
  template < typename Callable >
  Handler(Callable&& callable) : m_thread([&]()
  {
    for (std::queue<Item> queue; !m_isInterruptionRequested;)
    {
      {
        std::unique_lock<std::mutex> lock(m_mutex);
        m_queueIsNotEmpty.wait(lock, [&]{ return m_isInterruptionRequested || !m_queue.empty(); });
        m_queue.swap(queue);
      }
      for (; !queue.empty(); queue.pop())
        std::forward<Callable>(callable)(std::move(queue.front()));
    }
  }) { }

  void addItem(Item&& item)
  {
    {
      std::lock_guard<std::mutex> lock(m_mutex);
      m_queue.push(std::move(item));
    }
    m_queueIsNotEmpty.notify_one();
  }

  ~Handler()
  {
    m_isInterruptionRequested = true;
    m_queueIsNotEmpty.notify_all();
    m_thread.join();
  }

private:
  std::mutex              m_mutex;
  std::queue<Item>        m_queue;
  std::condition_variable m_queueIsNotEmpty;
  std::atomic<bool>       m_isInterruptionRequested = ATOMIC_VAR_INIT(false);
  std::thread             m_thread; // VM: Has to be the last field!
};

Any suggestions on what could be better in this implementation (e.g. a better way to handle the interruptions)?

Are there any hidden threats in this implementation that I don't see?

Small nitpicks

Naming: Handler is very non-descriptive. Maybe try Consumer instead?
Naming: addItem does not describe a Consumers action, consume or pass would be a better fit IMHO

Design problems
Handler is a queue fused with a single consumer, so there will never be more than one consumer for the queue!.
If the consumer is not required to run on its own thread, the implementation could be simplified to:
template<typename Item>
class SingleConsumer {
private:
    std::function<void(Item&&)> consumer;
    std::mutex mut; // can be removed if consumer is thread-safe on its own

public:
    template<typename Callable>
    SingleConsumer(Callable&& callable) : consumer(std::forward<Callable>(callable)) {}

    void consume(Item&& item) {
        std::unique_lock<std::mutex> lock(mut); // not necessary if consumer is thread-safe on its own
        consumer(std::move(item));
    }
};

On the other hand, if the requirement is that the consumer has to run on its own thread, why fuse it with its own queue? If there is only one queue, you could run multiple consumers on it to share the workload if needed and/or producers by themselves don't have to decide which consumer to call!
template<typename Item>
class ConcurrentQueue {
private:
    std::mutex mut;
    std::queue<Item> queue;

public:
    ConcurrentQueue() : queue() {}

    bool try_pop(Item& item) {
        std::unique_lock<std::mutex> lock(mut);
        if(queue.empty()) return false;
        item = queue.front();
        queue.pop();
    }

    void push(Item&& item) {
        std::unique_lock<std::mutex> lock(mut);
        queue.push(std::move(item));
    }
};

template<typename Item>
class Consumer {
private:
    std::thread consumer_thread;
    ConcurrentQueue<Item>& queue;
    std::atomic<bool> is_running = ATOMIC_VAR_INIT(true);

public:
    template<typename Callable>
    Consumer(ConcurrentQueue<Item>& q, Callable&& callable) : consumer_thread([callable]() {
        run(std::forward<Callable>(callable));
    }), queue(q) {}

    ~Consumer() {
        is_running = false;
        consumer_thread.join();
    }

    template<typename Callable>
    void run(Callable&& consumer) {
        Item item;
        while(is_running) {
            while(is_running && !queue.try_pop(item)) /* do nothing */;
            if(!is_running) return;
            consumer(item);
        }
    }
};

Of course, this example can be expanded according to your needs!


Finding the middle permutation This method finds all permutations of a string and stores them in a sorted array. It then returns the element in the middle of the array.

def middle_permutation(string)
  sorted = string.chars.to_a.permutation.map(&:join).sort
  sorted[sorted.length / 2 - 1]
end

For strings over 10+ characters this code is too slow. How can I speed it up?

Here are my tests. They pass, but I'd like it to be more efficient:

describe "Basic tests" do
  Test.assert_equals(middle_permutation("abc"),"bac")
  Test.assert_equals(middle_permutation("abcd"),"bdca")
  Test.assert_equals(middle_permutation("abcdx"),"cbxda")
  Test.assert_equals(middle_permutation("abcdxg"),"cxgdba")
  Test.assert_equals(middle_permutation("abcdxgz"),"dczxgba")
end

Calculating every permutation is very expensive so you want to avoid doing that. There are some ways to handle this:

1) Sort the characters before you run your algorithm. That way you are sorting a few characters not billions of permutations.

2) If the string has an even number of characters (say abcd) remove the almost middle character (b) and rewrite as something like 'b' + 'acd'.reverse

3) If the string has an odd number of characters (say abcde) you can remove the middle character (c) leaving abde and simplify the result as 'c' + middle_permutation('abcd')

(3) Will always be followed by (2)

So you could rewrite your code as:

def middle_permutation2(string)
  string = string.chars.sort.join("")
  return string if string.length <= 2

  if string.length.even?
    middle = string.length / 2 - 1
    remainder = string[0...middle] + string[middle+1..-1]
    string[middle] + remainder.reverse

  else
    middle    = string.length / 2
    remainder = string[0...middle-1] + string[middle+1..-1]
    string[middle] + string[middle-1] + remainder.reverse

  end

end


Editing HTML based on instructions in a data structure My getExtend() function, which will be included via require.js, has two inputs and one output:

  HTML file content
  config object which determine how to update the HTML, for example to create new script (as the first object in the array
  extendedHtmlObject ) or update existing script content such as
  attributes values)
  the output should be the extended HTML with all the modifications.

Here is an example config object.
Please notice that that in the first element of the array (there is scenario which the next attribute is not provided which say to put the new script at the bottom) the next said put the script tag after script that have ID "ui-boot"

var extendedHtmlObject = [{
                    type: 'script',
                    action: 'new',
                    content: 'console.log(‘hello world’);',
                    next: "ui-boot"
                }, {
                    type: 'script',
                    id: "ui-boot",
                    action: 'upd',
                    innerElem: [{
                        type: 'attr',
                        id: 'data--ui-comersion',
                        content: '1.17'
                    }, {
                        type: 'attr',
                        id: 'src',
                        content: '/test/test2/-co.js'
                    }]
                }

This is the main function:

getExtend: function(htmlContent, extendedHtmlObject) {
    var self = this;
    if (extendedHtmlObject) {
        extendedHtmlObject.forEach(function(configs) {
            switch (configs.type) {
                case 'script':
                    htmlContent = self._handleScriptElement(htmlContent, configs);
                    break;
                case 'head':
                    break;
            }
        });
    }
    return htmlContent;
},

This method determines if I need to create a new script or update existing script attributes according to the input object

_handleScriptElement: function(htmlFilecontent, configEntry) {

    var oExtendedHTML = htmlFilecontent;
    switch (configEntry.action) {
        case 'new':
            oExtendedHTML = this._createNewScript(htmlFilecontent, configEntry);
            break;
        case 'upd':
            var sParsedHtml = this._htmlParser(oExtendedHTML);
            oExtendedHTML = this._updateScript(oExtendedHTML, configEntry, sParsedHtml);
            oExtendedHTML = this._convertHtmlBack(oExtendedHTML);
            break;
    }
    return oExtendedHTML;
},

This is the method for creating new script with two option

  the first fork need to parse the html
  the second for doesn't.

_createNewScript: function(htmlFilecontent, configEn) {
    var sScriptContent = this._createScript(configEntry.content);
    if (configEn.next != null) {
        var sParsedHtml = this._htmlParser(htmlFilecontent);
        $(sScriptContent).insertAfter(sParsedHtml.find('#' + configEn.next));
        htmlFilecontent = this._convertHtmlBack(sParsedHtml);
    } else {
     //when the script is at the end of file
        var iHeadEndTagPos = htmlFilecontent.search("(| )*</(| )*head(| )*>");
        htmlFilecontent = htmlFilecontent.substr(0, iHeadEndTagPos) + sNewScript + htmlFilecontent.substr(iHeadEndTagPos);
    }
    return htmlFilecontent;
},

This code is redundant(_htmlParser & _convertHtmlBack ) and not efficient(I'm fairly new to JS and use it inside the create and update), could I maybe improve it with JS prototype?
I want to do the parse just once in the start and the parseBack at the end(of looping the input object) but the problem is that in the createNewScript the second fork doesn't need to use the parser.

update
If I can improve this question and add more info please let me know how, Im really need the code review community help since the code should go to prod and I want it to be more efficient

Personal preference, but I'd suggest 2-space indentation. You'll most likely nest a lot of stuff in JS, and using 4-space indentation can easily throw you over to the right side of your screen.

Another approach to reducing indentation overhead is to "return early" or offload the functionality to a separate function and use a ternary operation. This is especially when you have single if blocks with no else.

function foo(){
  var something = ''
  if(condition){
    something = doSomething();
  }
  return something;
}

// is the same as returning early:

function foo(){
  var something = ''
  if(!condition) return something;
  something = doSomething();
  return something;
}

// is the same as using a ternary:

function foo(){
  return condition ? doSomething() : '';
}

  This is the input: please notice that that in the firstobj of the array (there is scenario which the next attribute is not provided which say to put the new script at the bottom) the next said put the script tag after script that have ID "ui-boot"

When you have to explain your code, that's a sign that you have done something bad. In this case, you had to explain what next in the first element is, and id on the second element.

Being consistent in your objects as well as good naming schemes are a good start:

var extendedHtmlObject = [{
  action: 'new',
  type: 'script',
  properties: null,
  content: 'console.log(‘hello world’);'
  position: {
    anchor: '#ui-boot',
    placement: 'after'
  }
}, {
  action: 'update',
  type: 'script',
  properties: {
    id: 'ui-boot',
    src: '/test/test2/-co.js',
    'data--ui-comersion': '1.17',
  },
  content: '',
  position: null
}];

While there are legitimate use cases for a switch, it's usually a sign that indicates that some function is trying to do too many things. Also, switch can bloat easily and forgetting break is a danger waiting to happen. Consider using key-value pairs instead:

function actions(key, args){
  switch(key){
    case: 'foo':
      return doFoo(args);
      break;
    case: 'bar':
      return doBar(args);
      break;
    case: 'baz':
      return doBaz(args);
      break
  }
}

var value = actions(keyargs);

// is the same as

var actions = {
  foo: function doFoo(){...},
  bar: function doBar(){...},
  baz: function doBaz(){...}
};

var value = actions[key](args);

  could I maybe improve it with JS prototype?

If this is a singleton module, it doesn't make sense to use prototypes. Prototypes make sense when you create multiple instances that "inherit" the same methods. If you only create one instance, then it doesn't make sense to use a mechanism that was meant for building multiple instances of something.


Cadbury problem solution in Python Problem Statement
In a School, Chocolate bars have to be distributed to children waiting in a queue. Each Chocolate bar is rectangular in shape. Consider its side lengths are integer values.
The distribution procedure is as follows:-
If bar is not square in shape, then the largest possible square piece of Chocolate is broken and given to the first child in queue.
If bar is square in shape, then complete bar is given to the first child in queue.
Once a child receives his share of Chocolate, he leaves the queue. The remaining portion of the Chocolate bar is dealt in same fashion and the whole or a portion of it is given to the next child in the queue.
School has got a carton of Chocolate bars to be distributed among the children all over the School. The Chocolate bars in the carton are of different sizes. A bar of length i and breadth j is considered to be different from a bar of length j and breadth i. For every i such that \$M \le i \le N\$ and every j such that \$P\le j \le Q\$ (where M, N, P and Q are integers). Each Chocolate bar in carton is unique in length (i) and breath(j).
Given the values of M, N, P and Q (where M, N values are the ranges for length of Chocolate and P, Q values are the ranges for breadth of the chocolate). Find the number of children who will receive Chocolate from the carton.
Input Specification:
M, N, P, Q are of integer type (M, N values are the ranges for length of chocolate bar. P, Q values are the ranges for breadth of chocolate bar).
Output Specification:
Number of children who will receive Cadbury bar from the carton.
M = 5, N = 6, P = 3, Q=4 Here, i can be from 5 to 6 and j can be from 3 to 4. So the four bars will be in carton of sizes 5x3, 5x4, 6x3, 6x4.
First we choose a Cadbury bar of size 5x3 → first child would receive 3x3 portion ( remaining 2x3 portion ) → next child would receive 2x2 portion ( remaining 2x1 portion ) → now the remaining portion are 2 square pieces of (1x1), which can be given to 2 more children
So the Cadbury bar with the size of 5x3 can be distributed to 4 children.
Similarly we can find out number of children for rest of the combinations (i.e. 5x4, 6x3, 6x4) in the given range as follows

Can anyone suggest improvements?
m, n, p, q = raw_input().split(":")
ll = int
bb = int
count = 0

def cadbury(m, n, p, q):
    leng = [m, n]
    brd = [p, q]
    for l in leng:
        for b in brd:
            call(l, b)

def call(l, b):
    #print l,b
    global count
    area = l * b
    if l > b:
        bb = l
        rem = bb - b
        #print rem
        rem_part1 = rem
        #print rem_part1
        rem_part2 = b
        #print rem_part2
        l = rem_part1
        #print l
        b = rem_part2
        #print b
        if l != b:
            if (l==1) or (b==1):
                count += 1
                count += (l*b)
                #print count
                return
            else:
                count += 1
                #print count
                call(l, b)
        if l==b:
            count+=2
            #print count
            return
    elif b > l:
        ll = b
        #print ll
        rem = ll - l
        #print rem
        rem_part1 = rem
        #print rem_part1
        rem_part2 = l
        #print rem_part2
        l = rem_part1
        #print l
        b = rem_part2
        #print b
        if l != b:
            if (l==1) or (b==1):
                count += 1
                count += (l*b)
                #print count
                return
            else:
                count += 1
                #print count
                call(l, b)
        if l==b:
            count+=2
            #print count
            return
cadbury(int(m), int(n), int(p), int(q))
print count

I made it a little shorter:
The logic is also little more simple.

def TotalCount(M,N,P,Q):
    count=0
    for l in range(M,N+1):
        for b in range(P,Q+1):
            count +=CountPerChocolateBar(l,b)
    return count

Your cadbury function stayed almost the same. But your version has a bug: if the difference between M and N, or P and Q is not one (in your example it was one), your code fails:

a = 7
b = 10
for element in [a,b]:
  print (element)

result is 7, 10 . NOT the expected 7,8,9,10

def CountPerChocolateBar(l,b):

  count = 0
  while True:
      longerr=max(l,b)
      shorterr=min(l,b)
      count+=1
      diff=longer-shorter
      if diff==0:
        return count
      else :
        l=min(l,b)
        b=diff

Your call function can be simplified a little:
you can handle l>b ,b>l without branches, because it doesn't matter which one is bigger. You count the difference, increase the count variable, and based on the difference you return the count number or calculate the new b and l values. When the difference is 0, the while loop ends with the return statement

while True:

    numbers=raw_input("Number: ")

    M=int(numbers.split()[0])
    N=int(numbers.split()[1])
    P=int(numbers.split()[2])
    Q=int(numbers.split()[3])
    tc=TotalCount(M,N,P,Q)

    print (tc)

You can test my solution with this little loop. :)


Faster generation of list with multiple sublists I'm trying to improve the performance of some rather large code. One of the bottlenecks is the line below. It doesn't look like much but it is processed millions of times.

Is there any way to improve its performance at the code level (ie: no parallelization)? I'm perfectly fine with the final outcome being a numpy array instead of a list.

import numpy as np

def func(m1, m2):
    return -2.5 * np.log10(10 ** (-0.4 * m1) + 10 ** (-0.4 * m2))

# Generate data.
N1, N2 = 300, 200
aa = np.random.random((5, N1))
idxs = np.random.choice(N1, N2, replace=False)
bb = np.random.random((10, N2))

# I need to improve the performance of this line.
cc = [func(m[idxs], bb[i]) for (i, m) in enumerate(aa)]

Add

As Mibac suggested, the issue here might actually be the successive calls to func(), not the generation of the cc list itself.

If this is the case, then I'd need to improve the performance of the line:

-2.5 * np.log10(10 ** (-0.4 * m1) + 10 ** (-0.4 * m2))

or equivalently:

-2.5 * np.log10(c ** m1 + c ** m2)

where c=10**(-0.4).

aa = np.random.random((5, N1))
idxs = np.random.choice(N1, N2, replace=False)
bb = np.random.random((10, N2))

# I need to improve the performance of this line.
cc = [func(m[idxs], bb[i]) for (i, m) in enumerate(aa)]

Unless I'm misunderstanding something, half of bb is never used. Shouldn't the 5 in the instantiation of aa and the 10 in the instantiation of bb be the same value?

def func(m1, m2):
    return -2.5 * np.log10(10 ** (-0.4 * m1) + 10 ** (-0.4 * m2))

Algebraically, $$\log_{10}(10^x + 10^y) = \log_{10}(10^x (1 + 10^{y - x})) = x + \log_{10}(1 + 10^{y - x})$$ Since in this scenario \$-1 \le y - x \le 1\$ there doesn't seem to be any numerical-analytic reason not to simplify like this.

Depending on your error requirements, it may be possible to speed up the logarithm further by approximating it with a Taylor expansion $$\log_{10}(1 + 10^z) = \frac{\ln 2}{\ln 10} + \frac{z}{2} + \frac{\ln 10}{8} z^2 - \frac{\ln^3 10}{192} z^4 + \frac{\ln^5 10}{2880}z^6 - \ldots$$
or with a Padé approximant.


Would be possible to refactor this PHP code more? I really don't like the so many return statements. Anyone think that this could be refactored more?

public function validate($value, Constraint $constraint)
{
    if('' === $value || null === $value) {
        return;
    }

    if(!preg_match('/^[0-9]{7}([0-9]{3})[0-9]{1}$/', $value, $matches)) {
        $this->context->addViolation($constraint->message, array(
            '{{ value }}' => $value
        ));
        return;
    }

    // Fast Luhn check
    $vatin = (string) $matches[0];

    $luhn = array(0, 2, 4, 6, 8, 1, 3, 5, 7, 9);
    $sum  = 0;

    for($i = 0; $i < strlen($vatin); $i++) {
        $dgt  = (int) substr($vatin, $i, 1);
        $sum += (0 === $i % 2) ? $dgt : $luhn[$dgt];
    }

    if(0 !== ($sum % 10)) {
        $this->context->addViolation($constraint->message, array(
            '{{ value }}' => $value
        ));
        return;
    }

    // Province check
    $provinceCode = $matches[1];

    if(!in_array((int) $provinceCode, range(1, 121) + array(888, 999))) {
        $this->context->addViolation($constraint->provinceMessage, array(
            '{{ value }}' => $provinceCode
        ));
    }
}

Ah, the yoda syntax, don't see that too often. Anyways, on to the subject at hand. Here are a few ways you can refactor it, I'll address your specific question towards the end.

You can use the empty() function in your first comparison, it will return TRUE if the value is any FALSE value ('', 0, NULL, FALSE, "false", etc...), so it does the same thing but is a little cleaner.

if( empty( $value ) ) {
    return;
}

Another possible refactor, is to not use typecasting unless necessary. For instance, below you have a string typecast that is converting the element of an array to a string. This is unnecessary, at least if I'm reading the doc page on preg_match() properly. The 0th element will already contain a string, so this is redundant. Below that is another typecast that converts a string to an integer. This also seems unnecessary. PHP is a very loosely typed language, meaning transitioning from a integer to a string, or vice-versa, doesn't require any special type-casting or syntax. BTW: below each example is also another way to refactor the statement that may be a little better.

$vatin = (string) $matches[0];
$vatin = $matches[ 0 ];//the same
$vatin = array_shift( $matches );//avoids magic numbers, even though most will know where the 0 comes from

$dgt  = (int) substr($vatin, $i, 1);
$dgt = substr( $vatin, $i, 1 );//the same
$dgt = $vatin[ $i ];//string array syntax avoids the unnecessary function call

Avoid calling functions in for or while loop parameter lists. Both of these loops call any parameter functions on every iteration, meaning more overhead. Set the value before the loop. I'd also suggest doing the same in foreach loops, but its not as important.

$length = strlen($vatin);
for( $i = 0; $i < $length; $i++ ) {

Another problem you have here is that you are violating "Don't Repeat Yourself" (DRY) by using the same, or almost the same values in the addViolation() method so many times. A better way would be to set up a default, then log it once. There are probably a few different ways to go about this, but here's an example of one.

//in class
$violation = array(
    $constrain->message,
    $value
);

if( ! preg_match() ) {
    return $violation;
}
//repeat or return different violation as necessary

//implementation
$violation = $instance->validate( $value, $constraint );
if( is_array( $violation ) ) {
    list( $message, $value ) = $violation;

    $instance->context->addViolation( $message, array(
        '{{ value }}' => $value
    ) );
}

Now, the question you are asking about is more or less a style choice. The one-return vs many. At least if I am understanding your question correctly. I used to be of the first camp, but I think I'm leaning a little more towards the other now. Sometimes it makes sense to use a single return, but its a matter of context. For instance, if I found what I was looking for in the middle of a loop, its better to return from the loop early rather than finish it and then test a few other things. Your code can quite easily get skewed or convoluted or redundant when trying to maintain a single return.

So, while it may seem undesirable, your code is actually benefiting from these multiple return statement. I wouldn't try refactoring them out, but if you absolutely must, you can modify the $violation array I showed you above so that the value is only set if an error occured, then, at the end of your function, if it has been set you can return the array.

The only other solution I can think of, and perhaps a better one, would be to not use returns at all, but to throw errors instead. That appears to be very close to what you are doing anyways, so you might want to seriously contemplate this switch.


C single-linked list with remove Can you please point out any problems with this C forward list with remove node functionality?

I believe this can be way simpler ( removeNode function ) but I somehow cannot grasp it

#include <stdlib.h>
#include <stdio.h>
#include <assert.h>

struct node
{
  int val;
  struct node* next;
};
typedef struct node node;

node* newNode(int val)
{
  node* n = (node*)malloc(sizeof(node));
  n->val = val;
  n->next = NULL;
  return n;
}

void printList(node* head)
{
  if (head == NULL) return;

  do
  {
    printf("%d, ", head->val);
    head = head->next;
  } while (head->next);

  printf("\n");
}

void clean(node* head)
{
  while (head)
  {
    node* curr = head;
    if (head->next == NULL)
    {
      free(head);
      return;
    }

    head = head->next;
    free(curr);
  }
}

void insert(node* head, int val)
{
  while (head)
  {
    if (head->next)
    {
      head = head->next;
    }
    else
    {
      head->next = newNode(val);
      return;
    }
  }
}

void removeIf(node** head, int val)
{
  if (head == NULL) return;
  if (*head == NULL)
  {
    head = NULL;
    return;
  }

  while (*head && (*head)->val == val)
  {
    if ((*head)->next == NULL)
    {
      free(*head);
      *head = NULL;
      return;
    }
    node* tofree = *head;
    *head = (*head)->next;
    free(tofree);
  }

  node* curr = (*head)->next;
  node* prev = *head;

  while (curr)
  {
    if (curr->next == NULL) return;

    if (curr->val == val)
    {
      prev->next = curr->next;
      node* tofree = curr;
      curr = curr->next;
      free(tofree);
    }
    else
    {
      prev = prev->next;
      curr = curr->next;
    }
  }
}

int size(node* root)
{
  if (root == NULL) return 0;
  int sz = 1;
  while (root->next)
  {
    root = root->next;
    ++sz;
  }

  return sz;
}

void tests();

int main()
{
  tests();
}

/*
 * Here are the tests for lists functionalities
 */
void tests()
{
  {
    node* root = newNode(1);
    insert(root, 2);
    assert(size(root) == 2);
    clean(root);
  }
  {
    node* root = newNode(8);
    insert(root, 2);
    insert(root, 2);
    assert(size(root) == 3);
    clean(root);
  }
  {
    node* root = newNode(8);
    insert(root, 123);
    insert(root, 123);
    insert(root, 123);
    insert(root, 123);
    assert(size(root) == 5);
    clean(root);
  }
  {
    node* root = newNode(8);
    insert(root, 123);
    assert(size(root) == 2);
    removeIf(&root, 1);
    assert(size(root) == 2);
    clean(root);
  }
  {
    node* root = newNode(1);
    insert(root, 1);
    insert(root, 1);
    insert(root, 1);
    insert(root, 2);
    insert(root, 3);
    insert(root, 3);
    insert(root, 4);
    insert(root, 4);
    insert(root, 5);
    assert(size(root) == 10);
    removeIf(&root, 1);
    assert(size(root) == 6);
    clean(root);
  }
  {
    node* root = newNode(1);
    insert(root, 1);
    insert(root, 1);
    insert(root, 1);
    insert(root, 2);
    insert(root, 3);
    insert(root, 3);
    insert(root, 4);
    insert(root, 4);
    insert(root, 5);
    assert(size(root) == 10);
    removeIf(&root, 3);
    assert(size(root) == 8);
    clean(root);
  }
  {
    node* root = newNode(1);
    insert(root, 1);
    assert(size(root) == 2);
    removeIf(&root, 1);
    assert(size(root) == 0);
    clean(root);
  }
  {
    node* root = newNode(1);
    insert(root, 1);
    insert(root, 1);
    insert(root, 1);
    assert(size(root) == 4);
    removeIf(&root, 1);
    assert(size(root) == 0);
    clean(root);
  }
}

No need to peek ahead for the loop condition. Especially not AFTER incrementing it already. Coincidence that this function was not part of the test suite?

void printList(node* head)
{
  if (head == NULL) return;

  while(head)
  {
    printf("%d, ", head->val);
    head = head->next;
  };

  printf("\n");
}

Too much redundant code:

void clean(node* head)
{
  while (head)
  {
    node* curr = head;
    head = head->next;
    free(curr);
  }
}

In void insert(node* head, int val), no need to branch inside the loop. Do it step by step. First find the tail, then append.

void removeIf(node** head, int val) would become already much simpler if it did just add a temporary head to the list, as the special handling for removing a chain at the head wouldn't be necessary in that case. Finally destroy the temporary head again, and overwrite head unconditionally.

In int size(node* root), why call it root all of a sudden and no longer head?

Also, unnecessary lookahead and branching again:

int size(node* head)
{
  int sz = 0;
  while (head)
  {
    ++sz;
    head = head->next;
  }
  return sz;
}

In terms of code style only one remark, and that is on the location of the * when working with pointer types.

I know, placing it next to the type feels more natural, but it's semantically inaccurate, and it will trip you the first time you attempt to declare multiple variables at once. Placing it where there compiler would sort it helps preventing these mistakes.


Find all pairs of unique indices in a list such that the concatenation of the two words is a palindrome The task

  Given a list of words, find all pairs of unique indices such that the
  concatenation of the two words is a palindrome.

  For example, given the list ["code", "edoc", "da", "d"], return
  [(0, 1), (1, 0), (2, 3)].

const lst = ["code", "edoc", "da", "d"];

My functional solution

const findPalindromePairs = lst => {
  const isPalindrome = str => str === [...str].reverse().join("");
  return lst.reduce((pairs, w1, i) => {
    return [...pairs, ...lst.reduce((match, w2, j) => {
      if (i !== j && isPalindrome(`${w1}${w2}`)) { match.push([i,j]); }
      return match;
    }, [])];
  }, []);
};

console.log(findPalindromePairs(lst));

My imperative solution

function findPalindromePairs2 (lst) {
  const len = lst.length;
  const pairs = [];

  const isPalindrome = str => str === [...str].reverse().join("");
  for (let i = 0; i < len; i++) {
    for (let j = 0; j < len; j++) {
      if (i !== j && isPalindrome(lst[i] + lst[j])) { pairs.push([i,j]); }
    }
  }
  return pairs;
}

console.log(findPalindromePairs2(lst));

I don't really see anything that jumps out as an obvious simplification. I looked online for other code for this task and only found one in python which uses the same basic algorithm.

The only suggestion I have is for readability, the name match in the functional solution might be slightly confusing for anyone reading the code. Perhaps a more appropriate name would be something like subMatches.

I did notice that the functional solution concatenates the strings with a template literal, while the imperative solution uses traditional string concatenation. Why not just use the traditional technique in the functional solution? The results of this jsPerf show negligible differences between the two approaches.


Sum of primes under a certain number I have tried two different methods to find the sum of primes under a certain number. However both of these methods end up taking a very long time for large numbers. How can I optimise either of these two methods?

Method 1 (Sieve of Eratosthenes):

import time
import math

primes_in_range = [2]

n = int(input("Enter the range of your sieve"))

start = time.time()

for x in range(3, n+1, 2):
    primes_in_range.append(x)

d = 3

while d <= math.sqrt(n):
    if d in primes_in_range:
        for j in range(2 * d, n + 1, d):
            if j in primes_in_range:
                primes_in_range.remove(j)
    d += 2

print(sum(primes_in_range))
end = time.time()
print(end - start)

Method 2:

import time
import math

def is_prime(n):
  if n>1:
    for i in range (2, int(math.sqrt(n)) + 1):
      if (n % i) == 0:
        return False
    else:
      return True
  else:
    return False

n = int(input("Enter the range of your sieve"))

start = time.time()

total = 2

i = 3

while i < n:
    if is_prime(i):
        total += i
    i += 2

print(total)

end = time.time()
print(end -start)

Method 1 (Sieve of Eratosthenes) is definitely the faster method to use.

Starting point on my computer, summing primes less than 100,000 takes 39.916 seconds.

List to Set

The problem is basically this line:

primes_in_range = [2]

This creates a list, to which you add all odd numbers above 1, and then repeatedly search for existence of a number within (d in primes_in_range),  then for all multiples of that number, search for the multiple (j in primes_in_range), and then search for it a second time in the process of removing it (primes_in_range.remove(j)).  Searching a list is an \$O(n)\$ operation.  When you do it approximately once for every number in the list, your algorithm devolves to \$O(n^2)\$.

On top of this, you’re computingmath.sqrt(n) many, many times - a relatively expensive operation - yet the value does not change.  You only need to compute it once.

The second item you can solve by computing the \$\sqrt n\$ once, and store it in a local variable.

For the first issue, there are a few possibilities.  One, you could use a set, instead of a list.  A set has approximately \$O(1)\$ lookup time, which is much faster than \$O(n)\$.

primes_in_range = { 2 }

Since sets are not ordered, you don’t "append()" items to it, you "add" them instead.

Time has now dropped to 0.020511 seconds

Since the time has dropped to under a second, let's increase to primes less than 10,000,000 for better time measurements.

Time for summing primes less than 10,000,000: 3.519 seconds

Search to Remove

When removing, you are checking for the existence of the item in primes_in_range before removing it, since removing it when it is not present will raise an exception.  The set class has a discard method, which will only remove an item if present.  No need to check for it to be present ahead of time.

Time for summing primes less than 10,000,000: 3.453 seconds

Removing Multiples

When you find a prime, d, you cross off multiples of that prime, starting at 2*d, and going 3*d, 4*d, 5*d, and so on.  But when you determined 3 was a prime number, you crossed off all multiples of 3, so crossing off 3*d is a waste of time, for d > 3.  Similarly, crossing off multiples of the prime 5 means crossing off 5*d for d > 5 is also a waste of time.  The first multiple of d that hasn't been crossed off will be d*d, so you can remove starting at that point:

        for j in range(d * d, n + 1, d):
            primes_in_range.discard(j)

Time for summing primes less than 10,000,000: 3.305 seconds

Similarly, since you are only considering odd prime candidates in your sieve, you don't need to cross off any even multiples.  Starting at d*d, the next multiple you need to remove would be (d+2)*d, not (d+1)*d.  Your step size can be 2*d.

        for j in range(d * d, n + 1, 2 * d):
            primes_in_range.discard(j)

Time for summing primes less than 10,000,000: 2.105 seconds

Loop Like a Native

Python has its own looping, which includes incrementing loop indexes.  This native Python looping is usually faster than writing loops with their own index manipulation:

primes_in_range = {2}
primes_in_range.update(x for x in range(3, n+1, 2))

for d in range(3, math.isqrt(n) + 1, 2):
    if d in primes_in_range:
        primes_in_range.difference_update(j for j in range(d * d, n + 1, 2*d))

Here, constructing the set is done with a generator expression in primes_in_range.update(...), and removing prime multiples is done with a generator expression in primes_in_range.difference_update(...) which adds and removes entire swathes of items in a single operation:

Time for summing primes less than 10,000,000: 2.049 seconds

Direct Indexing

However, a set is still an expensive memory structure to maintain.  It requires hashing the values which are being added, creating bins to store the values in, rebinning as the set size changes, and so on.  An array of flags is much more efficient, time-wise:

primes_in_range = [False] * n
primes_in_range[2] = True
for x in range(3, n, 2):
    primes_in_range[x] = True

for d in range(3, math.isqrt(n) + 1, 2):
    if primes_in_range[d]:
        for j in range(d * d, n + 1, 2 * d):
            primes_in_range[j] = False

total = sum(idx for idx, flag in enumerate(primes_in_range) if flag)

The first line, above, allocates an array of n items, each containing the flag value False.  Then, indexing into the array is an \$O(1)\$ operation.  Set the flag to True for all odd numbers above 3, and then proceed to set the appropriate flags to False based on the Sieve of Eratosthenes algorithm.

Time for summing primes less than 10,000,000: 1.398 seconds

Bit Array

That last approach is a little inefficient when it comes to memory.  It requires a list of n 28-byte items to store one True/False flag.  These flags could be packed 8 to a byte, resulting in a 99.5% reduction in required memory!  The bitarray package provides such an array of flags.

from math import isqrt
from bitarray import bitarray

def sum_of_primes_below(n):
    primes_in_range = bitarray(n)
    primes_in_range.setall(False)

    primes_in_range[2] = True      # 2 is a prime
    primes_in_range[3::2] = True   # Odd numbers starting at 3 are prime candidates

    for d in range(3, isqrt(n) + 1, 2):
        if primes_in_range[d]:
            primes_in_range[d*d::2*d] = False    # Reset multiples of prime candidate

    return sum(idx for idx, flag in enumerate(primes_in_range) if flag)

Time for summing primes less than 10,000,000: 0.455 seconds


Iterator based version of quicksort, allowing user defined pivot-selection-strategy I have implemented a iterator based version of Quicksort, which allows the user to define and pass his own pivot-selection-strategy:

template<typename It>
It Partition(It begin,It end,It pivot)
{
    std::swap(*begin, *pivot);
    pivot = begin;
    It i{ begin }, j = std::next(begin, 1);
    for(;j != end; j++)
    {
        if(*j < *pivot)
            std::swap(*j, *++i);
    }
    std::swap(*pivot, *i);
    return i;
}

template<typename It, typename Pivot>
void Quicksort(It begin, It end, Pivot&& GetPivot)
{
    if (std::distance(begin, end) == 0)
        return;
    It pivot{ GetPivot(begin,end) };
    It elementAtCorrectPosition = Partition(begin, end,pivot);
    Quicksort(begin, elementAtCorrectPosition, GetPivot);
    Quicksort(std::next(elementAtCorrectPosition, 1), end, GetPivot);
}

The algorithm can be used in the following way:

std::vector<int> v{  3, 8, 2, 5, 1, 4, 7, 6 };
auto quickSort = v;
auto stdSort = v;
Quicksort(quickSort.begin(),
          quickSort.end(),
          [](auto a, auto b) { return a; });
std::sort(stdSort.begin(), stdSort.end());
Assert::IsTrue(quickSort == stdSort);

Where the lambda expression

 [](auto a, auto b) { return a; }

is just to demonstrate how a pivot selection strategy can be passed to the algorithm.

Any suggestions that lead to a more robust, concise and readable code are welcome.

Special thanks to @ratchet freak, who discovered an emberassing bug:

The initial implementation only worked for the special case, where the first element is selected as pivot. See above for the corrected version.

One thing that is missing is selecting a custom comparison function. This is usually done by adding a std::less like type to the template and passing that on to the partition

template<typename It, typename Compare>
It Partition(It begin,It end,It pivot, Compare comp)
{
    std::swap(*begin, *pivot);
    pivot = begin;
    It i{ begin }, j = std::next(begin, 1);
    for(;j != end; j++)
    {
        if(comp(*j, *pivot))
            std::swap(*j, *++i);
    }
    std::swap(*pivot, *i);
    return i;
}


Coding - Find a bitwise XOR for a range of numbers in java Find a bitwise XOR for a range for.e.g (5,8) --> 5 bitXOR 6 | 7 bitXOR 8 3 bitXOR 15 12

Expected worst time Complexity - O(log(n))

Expected worst space Complexity - O(1)

I have written the below code, could you please help me to improve it?

static List<Integer> list;
    public static void main(String[] args) {
        int bitXORProduct = solution(5,8);
        System.out.println(bitXORProduct);
    }
    public static int solution(int M, int N) {
        if (isValidatedInput(M, N)) {
            int lastXOR = M;
            int currentXOR = 0;
            try {
                for (int i = M; i < N; i++) {
                    currentXOR = computeXOR(lastXOR, i + 1);
                    lastXOR = currentXOR;
                }
            } catch (Exception e) {
                System.out.println("Error Found : -" + e);
            }
            return lastXOR;
        }
        System.out.println("Input is not in the range or valid");
        return -1;
    }
    private static boolean isValidatedInput(int M, int N) {
        if (0 <= M && 0 <= N && M <= Math.pow(10, 9) && N <= Math.pow(10, 9) && M <= N) {
            return true;
        } else
            return false;
    }
    private static Integer computeXOR(Integer m, Integer n) {
        return m ^ n;
    }

First of all the solution you have given is complexity O(n), not O(log(n)). It is possible to do it in O(log(n)) by calculating the result bit by bit. For example, suppose I have a range of four consecutive integers. Then I know that two of those numbers are odd and two of those numbers are even. Since only odd numbers have the first bit set I already know that XOR-ing the four numbers will leave the first bit at 0. For the second bit, I notice that for each set of four consecutive numbers there is always one number 2 mod 4 and one number 3 mod 4. Since all integers which have the second bit set ar either 2 or 3 mod 4, I know that XOR-ing the set of four integers will leave the second bit at 0. In this way I can determine for each bit if it should be set or not by looking at the properties of the numbers in the range. Usually this can be done in O(1) by only using the boundries of the range. Since there are log2(n) rounded up bits in n, this solution will have complexity O(log(n)).

On to the code.

static List<Integer> list;

This list is not needed, it can be removed.

public static int solution(int M, int N)

The name of the method is solution but it doesn't describe what it is doing. Try to give it a name in accordance with it's function, something like computeXorForRange perhaps. Also, all other variables and methods in your code are camelCase so for consistency you should make M and N lower case. Or you can rename them to lowerBound and upperBoundInclusive if you like to tell the reader what they represent.

int lastXOR = M;
currentXOR = 0;
try {
  for (int i = M; i < N; i++) {
    currentXOR = computeXOR(lastXOR, i + 1);
    lastXOR = currentXOR;
  }
} catch (Exception e) {
  System.out.println("Error Found : -" + e);
}

In this part it is not needed to have to different variables, currentXOR and lastXOR. One should be enough. Having the two variables here makes it more complex than it should be. Also, why are you catching exceptions here? What exceptions do you expect to happen? Since the code used in the try does not throw any exception except for the possible standard java runtime exceptions like for example OutOfMemoryException, it is not needed to have a try catch.

System.out.println("Input is not in the range or valid");
return -1;

This part is at the end of the method. Instead you can negate the if statement at the beginning of the method and put this piece in the if. Then it becomes more clear what happens if the conditions are not met. There is also a return -1 which is more of a C style thing. In this case it is perfectly fine to use an Exception, the IllegalArgumentException will fit since the arguments passed are not valid (they are too small or too large).

private static boolean isValidatedInput(int M, int N) {
  if (0 <= M && 0 <= N && M <= Math.pow(10, 9) && N <= Math.pow(10, 9) && M <= N) {
    return true;
  } else
    return false;
}

The name of the method can be changed to isValidInput. The name isValidatedInput suggests that there will be a check if the input is already validated while it seems you are validating the input. There us also no reason to have the return true and return false, you can immediatly return the condition used in the if. Because if that condition is true the method will then return true and if the condition is false the method will then return false. Lastly, the upper bound of 10^9 seems arbitrary. If you want to prevent overflows and underflows you can use Integer.MAX_VALUE as upper bound.

private static Integer computeXOR(Integer m, Integer n) {
  return m ^ n;
}

In the rest of the code you use int yet here you use Integers. Using Integer is not necessary in this case since you don't have to deal with null values so using int would be consistent with the rest of the code. You can also ask if this method is really needed. IMO something like x ^ y is perfectly fine and clear and you don't need a separate method for that, but if it helps you, you can leave it in.

All in all with the above suggestions the code will become

public static void main(String[] args) {
  int bitXORProduct = solution(5,8);
  System.out.println(bitXORProduct);
}

public static int solution(int lowerBound, int upperBoundInclusive) {
  if (!isValidInput(lowerBound, upperBoundInclusive)) {
    throw new IllegalArgumentException("Input is not in the range or valid");
  }

  int currentXOR = lowerBound;
  for (int i = lowerBound; i < upperBoundInclusive; i++) {
    currentXOR = currentXOR ^ (i + 1); // this can also be written as currentXOR ^= i + 1;
  }

  return currentXOR;
}

private static boolean isValidInput(int M, int N) {
  return 0 <= M
      && 0 <= N
      && M <= Math.pow(10, 9)
      && N <= Math.pow(10, 9)
      && M <= N;
}
```


Repository to update flights I have the Repository below, but I have noticed that there is a recurrent pattern try / catch which does not seem to be that clean, is there a better way to achieve or to extract that sort logic elsewhere?

public class FlightRepository : IFlightRepository
{
    private readonly FlightManagerDbContext _context;

    public FlightRepository(FlightManagerDbContext context)
    {
        _context = context;
    }

    public Result CreateFlight(Flight flight)
    {
        try
        {
            var tmp = _context.Flight.FirstOrDefault(f => f.Code == flight.Code);
            if (tmp != null)
                return Result.Conflict($"Flight {flight.Code} already exists");
            _context.Flight.Add(flight);
            _context.SaveChanges();
            return Result.Ok("Flight successfully saved");
        }
        catch (Exception e)
        {
            return Result.Fail(new List<string>() {$"An error occured, {e.Message}"});
        }
    }

    public Result UpdateFlight(Flight flight)
    {
        try
        {
            var tmp = _context.Flight.FirstOrDefault(f => f.Code == flight.Code);
            if (tmp == null)
                return Result.NotFound($"Flight { flight.Code} not found");

            var toUpdate = _context.Flight.First(f => f.Code == flight.Code);
            toUpdate.Distance = flight.Distance;
            toUpdate.DepartureLatitude = flight.DepartureLatitude;
            toUpdate.DepartureLongitude = flight.DepartureLongitude;
            toUpdate.DepartureName = flight.DepartureName;
            toUpdate.DepartureTime = flight.DepartureTime;
            toUpdate.ArrivalLatitude = flight.ArrivalLatitude;
            toUpdate.ArrivalLongitude = flight.ArrivalLongitude;
            toUpdate.ArrivalName = flight.ArrivalName;
            toUpdate.ArrivalTime = flight.ArrivalTime;
            toUpdate.ConsumptionPerKm = flight.ConsumptionPerKm;
            toUpdate.TakeOffEffort = flight.TakeOffEffort;
            toUpdate.FlightTime = flight.FlightTime;
            toUpdate.FuelNeeded = flight.FuelNeeded;

            _context.SaveChanges();

            return  Result.Ok($"Flight {flight.Code} successfully updated");
        }
        catch (Exception e)
        {
            return Result.Fail(new List<string>() {$"An error occured, {e.Message}"});
        }
    }

    public Result<Flight> SelectFlight(string code)
    {
        try
        {
            var flight = _context.Flight.FirstOrDefault(f => f.Code == code);
            if (flight == null)
                return Result<Flight>.NotFound($"Flight {code} not found");
            return  Result<Flight>.Ok(flight);
        }
        catch (Exception e)
        {
            return Result<Flight>.Fail(new List<string>(){$"An error occured, {e.Message}"});
        }
    }

    public Result<List<Flight>> SelectFlights()
    {
        try
        {
            return Result<List<Flight>>.Ok(_context.Flight.Any()
                ? _context.Flight.ToList()
                : new List<Flight>());
        }
        catch (Exception e)
        {
            return Result<List<Flight>>.Fail(new List<string>() {$"An error occured,{e.Message}"});
        }
    }
}

The repository does not need to take responsibility for the errors - remove them from here. The flight repository should add CRUD operations to Flight Dto and not return a result - remove the result. Validation should be done before calling the methods. You are not an automapper, use a tool. You do not need to invent a bicycle every time you need a new repository, use generics. Additionally, you do not need to repeat Flight in every method name, they exist in the Flight repository you can infer that from context.
All error messages should be unique.

For example, implement an interface:

public interface IRepository<T> where T : EntityBase
{
    T GetById(int id);
    IEnumerable<T> List();
    void Add(T entity);
    void Delete(T entity);
    void Edit(T entity);
}

public abstract class EntityBase
{
   public int Id { get; protected set; }
}

For more info: https://deviq.com/repository-pattern/

The model that contains the list of flights should contain the validation rules and validate flights or the model of the Flight should validate itself.

What you get from the database is not a Flight model, but a dto. You should use a translator to create a model from dto. Dto's should not contain any business logic just data. Clear separation of concerns helps is needed for clear code.


config files for different development environments (Production, Dev, Testing) I am following a guide that specifies how to manage different configurations (DB_URI, LOGGER_NAME, etc...) per development environments.

I create a config module that is structured like this

\my_project\config\__init__.py

import os
import sys
from . import settings

# create settings object corresponding to specified env
APP_ENV = os.environ.get('APP_ENV', 'Dev')
_current_config = vars(setting).get(f'{APP_ENV}Config')

# copy attributes to the module for convenience
for atr in [f for f in dir(_current_config) if '__' not in f]:
    # environment can override anything
    val = os.environ.get(atr, getattr(_current_config, atr))
    setattr(sys.modules[__name__], atr, val)

\my_project\config\settings.py

class BaseConfig:
    DEBUG = True
    LOGGER_NAME = 'my_app'

class DevConfig(BaseConfig):
    # In actuality calling a function that fetches a secret
    SQLALCHEMY_DATABASE_URI = 'sqlite://user.db'

class ProductionConfig(BaseConfig):
    DEBUG = False
    # In actuality calling a function that fetches a secret
    SQLALCHEMY_DATABASE_URI = 'sqlite://user.db'

calling the config

application.py

import config
config.SQLALCHEMY_DATABASE_URI

This approach is kind of a hit and miss for me.
on one hand it allows me to easily specify several environments (dev, stage, prod, dev) without having a complex if else logic in my settings.py.

On the other hand I dont like using setattr on sys.modules[__name__], seems a bit wonky to me. what do you think ?

Incase you are using Docker for deployment. The required environment variable can be configured as Kuberenetes configuration and it reads based on the deployment configuration environment.


A better way to do LINQ projections I was wondering if there's a better way of doing LINQ projections than what I'm doing below. This method is one of many methods creating a view of an object tree.

The first thing that I don't like is that I'm returning an anonymous object. Should I create a BreakdownReport class or something else instead?

Also, the copying of property values from right to left is a bit of a hassle.

public object GetBreakdownsReport()
{
    return CurrentSecurity
        .Breakdowns
        .SelectMany(b => b.BreakdownLines, (b, bdl) => new
        {
            AccountId = b.Account.Id,
            Structure = b.Account.Structure,
            Market = b.Account.Market,
            BeneficiaryId = bdl.Beneficiary.Id,
            BeneficiaryType = bdl.Beneficiary.BeneficiaryType,
            CountryCode = bdl.Beneficiary.CountryCode,
            CountryName = bdl.Beneficiary.CountryName,
            DtaRate = bdl.Beneficiary.DtaRate,
            HoldingCode = b.HoldingCode,
            InstructionAmount = bdl.InstructionAmount,
            NarrativeText = bdl.Narrative.Text,
            Chapter = bdl.Narrative.Chapter,
            Paragraph = bdl.Narrative.Paragraph,
            Subparagraph = bdl.Narrative.Subparagraph,
            TaxRate = bdl.Narrative.TaxRate,
        })
        .ToList();
}

There's nothing wrong with this kind of assignment logic per se, but you usually don't want it cluttering up the rest of your application logic so it usually belongs in a constructor method. If you're using the GetBreakDownsReport() method to construct the source for a datagrid, replace your anonymous object with a BreakdownsReportViewModel object and make a constructor that handles all property assignments. This will have the added benefit of making your GetBreakdownsReport() method signature more expressive and easier to read.