Remove Empty String array from ArrayList I am trying to remove empty a String array from ArrayList:

  List<String[]> someList = new ArrayList<String[]>();

which contains data like:

[
  { "Loic" , "Remy" , "Fail","Medical"} ,
  { "Faser" , "Foster" , "Pass","Southampton","GK","Young"} ,
  { "" , "" ,} ,
  { "" , "" , "","","",""} ,
  { "Emre" , "Can" , "Pass","Liverpool","CDM"}
]

I want to remove the empty String[] from the ArrayList:

List<String[]> someList = (List<String[]>) csvMappedData.get(Constants.CSV_DATA);
    //Clone to new Array List
    List<String[]> cloneCSV = new ArrayList<String[]>(someList);
    for (String[] csvSingleLine : cloneCSV) {
        //Create New List of String only
        List<String> strings = new ArrayList<String>(Arrays.asList(csvSingleLine));
        //Remove all string that are null or blank
        strings.removeAll(Arrays.asList(null, ""));
        //Check if the List is empty or null after removing in above step
        if (strings.isEmpty() || Validator.isNull(strings)) {
            //If yes it is all blank String array
            //Remove from orginal LIST
            someList.remove(csvSingleLine);
        }
    }

The above code is working fine for me.

Questions:

Is there any other alternative solution that is quick and elegant?
What are the change that I could make to it, to make it more
efficient?

The size of each row varies, so I couldn't use remove all by creating a dummy array.

There are a few tricks you can use to improve this code.

First of all,

List<String[]> someList = (List<String[]>) csvMappedData.get(Constants.CSV_DATA);
List<String[]> cloneCSV = new ArrayList<String[]>(someList);
for (String[] csvSingleLine : cloneCSV) {

I understand that you're using this "copy the list"-approach to avoid a ConcurrentModificationException, but instead of doing that you can use an Iterator.

List<String[]> someList = (List<String[]>) csvMappedData.get(Constants.CSV_DATA);
Iterator<String[]> iterator = someList.iterator();
while (iterator.hasNext()) {
    String[] strings = iterator.next();
    if (stringsShouldBeRemoved) {
        iterator.remove();
    }
}

Now, as for your code to check for whether to remove a String[]:

List<String> strings = new ArrayList<String>(Arrays.asList(csvSingleLine));
strings.removeAll(Arrays.asList(null, ""));
if (strings.isEmpty() || Validator.isNull(strings)) {
    someList.remove(csvSingleLine);
}

Again, you're using a copying-approach. To check for whether or not an object fulfills a criteria, you don't need to copy it, modify it, and check if it fulfills another criteria. Copying it and modifying it makes it slower and uses more memory.

Instead, let's use a method:

boolean shouldRemoveCSVSingleLine(String[] csvSingleLine) {
    for (String str : csvSingleLine) {
        if (str != null && !str.equals("")) {
            return false;
        }
    }
    return true;
}

Now you can use this method in the while(iterator.hasNext()) loop:

List<String[]> someList = (List<String[]>) csvMappedData.get(Constants.CSV_DATA);
Iterator<String[]> iterator = someList.iterator();
while (iterator.hasNext()) {
    String[] strings = iterator.next();
    if (shouldRemoveCSVSingleLine(strings)) {
        iterator.remove();
    }
}


Number of capture groups in a regular expression in php I am trying to find the number of capture groups for a regular expression using this PHP function.

/**
 * Finds the number of groups in a regular expression
 * @param $regexPatten string The regular expression
 * @return int Returns the number of groups inside the regular expression
 */
public static function numOfGroups($regexPatten)
{
    $regexPatten = str_replace('\\\\', "", $regexPatten); // remove all escaped backslashes
    $regexPatten = str_replace('\\(', "", $regexPatten); // remove all escaped open parentheses
    $regexPatten = str_replace('(?:', "", $regexPatten); // remove all none capture groups

    return substr_count($regexPatten, "("); // count the remaining opening parentheses
}

And here is my test:

var_dump(Parser::numOfGroups("(test)")); // 1  --  will match: test
var_dump(Parser::numOfGroups("\((test)\)")); // 1  --  will match: (test)
var_dump(Parser::numOfGroups("\\\\\((test)\\\\(\d+)\)")); // 2  --  plain regex: \\\((test)\\(\d+)\)  --  will match: \(test\343)
var_dump(Parser::numOfGroups("((?:test(?: )?)+)")); // 1  --  will match: test test

Is there any use cases I have forgotten?

You might consider non-capturing groups

(?:/*pattern*/)

These will match your regex but won't normally be operated on in whatever code is running the regex you're evaluating.


Outputting a 2^n x 2^n matrix I've written a simple program in Python which does the following:

Input: an integer \$n\$

Output: a \$2^n x 2^n\$ matrix containing small \$L\$s which fill the entire matrix except one single square. Each L has its unique number, and consists of three squares.

It's a fairly simple program, and you can see clearly what it does by typing in a few different \$n\$s.

What I'd like to do: Keep the actual algorithm as it is now (the rec() function), but improve my C-like code to make full use of Python's features.

#!/usr/bin/env python

import sys, math

ur = [[1,0],
      [1,1]]

ul = [[0,1],
      [1,1]]

lr = [[1,1],
      [1,0]]

ll = [[1,1],
      [0,1]]

shapeCounter = 1

matrix = []

def printMatrix():
    for row in matrix:
        for element in row:
            if(element >= 10):
                print("%d" % element),
            else:
                print(" %d" % element),
        print
    print

def square(n):
    global matrix
    matrix = [[0]*(2**n) for x in xrange(2**n)]
    matrix[0][len(matrix) - 1] = -1
    rec(0, 0, len(matrix) - 1, len(matrix) - 1)
    matrix[0][len(matrix) - 1] = 0
    printMatrix()

def writeRect(rect, x, y):
    global shapeCounter
    matrix[y][x] += rect[0][0] * shapeCounter
    matrix[y+1][x] += rect[1][0] * shapeCounter
    matrix[y][x+1] += rect[0][1] * shapeCounter
    matrix[y+1][x+1] += rect[1][1] * shapeCounter
    shapeCounter += 1

def rec(sx, sy,
        ex, ey):
    if(ex - sx < 1 or ey - sy < 1):
        return
    #print("(%d,%d) (%d,%d)" %(sx,sy,ex,ey))

    if(matrix[sy][sx] != 0):
        rect = ul
    elif(matrix[ey][sx] != 0):
        rect = ll
    elif(matrix[sy][ex] != 0):
        rect = ur
    elif(matrix[ey][ex] != 0):
        rect = lr

    hx = sx + (ex - sx) / 2
    hy = sy + (ey - sy) / 2

    writeRect(rect, hx, hy)

    rec(sx, sy, hx, hy)
    rec(hx + 1, sy, ex, hy)
    rec(sx, hy + 1, hx, ey)
    rec(hx + 1, hy + 1, ex, ey)

if __name__=='__main__':
    square(int(sys.argv[1]))

#!/usr/bin/env python

import sys, math

ur = [[1,0],
      [1,1]]

ul = [[0,1],
      [1,1]]

lr = [[1,1],
      [1,0]]

ll = [[1,1],
      [0,1]]

Rather then creating four separate variables, I'd suggest making a list. I'd also suggest using the numpy library for its array type. It makes working with 2D arrays much nicer then using lists of lists.

shapeCounter = 1

matrix = []

Don't use global variables. Instead, pass matrix in as the parameter to the function.

def printMatrix():
    for row in matrix:
        for element in row:
            if(element >= 10):

Parens aren't needed here

                print("%d" % element),
            else:
                print(" %d" % element),
        print
    print

Note that if you used numpy, that library would already be able to nicely print a matrix.

def square(n):
    global matrix

This function should really create and return the matrix

    matrix = [[0]*(2**n) for x in xrange(2**n)]

In numpy, this would be matrix = numpy.zeros( (2**n, 2**n) )

    matrix[0][len(matrix) - 1] = -1
    rec(0, 0, len(matrix) - 1, len(matrix) - 1)
    matrix[0][len(matrix) - 1] = 0
    printMatrix()

Functions that do work on your variables shouldn't print them. Your main control function should do the printing.

def writeRect(rect, x, y):
    global shapeCounter

This isn't a terrible use of a global, but I'd avoid it. I'd have an object this attribute was stored on.

    matrix[y][x] += rect[0][0] * shapeCounter
    matrix[y+1][x] += rect[1][0] * shapeCounter
    matrix[y][x+1] += rect[0][1] * shapeCounter
    matrix[y+1][x+1] += rect[1][1] * shapeCounter
    shapeCounter += 1

def rec(sx, sy,
        ex, ey):

I wouldn't use abbreviations. Use start_x, end_y to make your code clearer.

    if(ex - sx < 1 or ey - sy < 1):
        return
    #print("(%d,%d) (%d,%d)" %(sx,sy,ex,ey))

    if(matrix[sy][sx] != 0):
        rect = ul
    elif(matrix[ey][sx] != 0):
        rect = ll
    elif(matrix[sy][ex] != 0):
        rect = ur
    elif(matrix[ey][ex] != 0):
        rect = lr

You don't need all those parens

    hx = sx + (ex - sx) / 2
    hy = sy + (ey - sy) / 2

    writeRect(rect, hx, hy)

    rec(sx, sy, hx, hy)
    rec(hx + 1, sy, ex, hy)
    rec(sx, hy + 1, hx, ey)
    rec(hx + 1, hy + 1, ex, ey)

if __name__=='__main__':
    square(int(sys.argv[1]))


Laravel controller for creating payments with Mollie I have a controller with functions to create and finish a Mollie payment. But I've got the idea that those functions are too big and do too many things. What I understand is that a function should do only 1 thing.

Any tips on how I can tidy this further up? Is it maybe a good moment to use events? Or are there any better solutions?

Note: creditBundles() returns an array with possible packages/bundles which are currently for sale.

public function __construct()
{
    $this->payment = Mollie::api()->payments();
}

public function createPayment(Request $request, $bundle)
{
    $bundles = $this->creditBundles();

    if (!array_key_exists($bundle, $bundles)) {
        return redirect()->route('order.credits');
    }

    $order_id = time();
    $bundle = $bundles[$bundle];

    $payment = $this->payment->create([
        "amount"            => $bundle['price'],
        "description"       => $bundle['credits'] . " credits",
        "redirectUrl"       => route('order.credits.finish', ['order_id' => $order_id]),
        "webhookUrl"      => route('order.credits.webhook'),
        "metadata"          => ['order' => 'credits', 'order_id' => $order_id],
    ]);

    CreditOrder::create([
        'user_id' => auth()->user()->id,
        'order_id' => $order_id,
        'payment_id' => $payment->id,
        'payment_status' => $payment->status,
        'credits' => $bundle['credits'],
    ]);

    return redirect($payment->getPaymentUrl());
}

public function finish(CreditOrder $order)
{
    $user = auth()->user();

    if ($order->user_id !== $user->id) {
        return redirect()->route('order.credits');
    }

    $payment = $this->payment->get($order->payment_id);

    if (!$payment->isPaid() || $order->payout) {
        return view('order_status', [
            'payment' => $payment,
            'order' => $order,
        ]);
    }

    $user->increment('credits', $order->credits);
    $order->payout = true;
    $order->save();

    notify()->flash('Thanks ' . escape($user->first_name) . '!', 'success', [
        'text' => 'We received your payment and added ' . $order->credits . ' credits to your account!'
    ]);

    return redirect()->route('home');
}

Yes, the Single Reponsibility Principle can apply to methods (I know that article is geared towards Swift but should hopefully still be practical in other languages).

While there are guidelines like this answer:

  What's the Right Size, Really?

  Depends on the language you use, but in general (and for my personal taste):

  Ideally, less than 25 lines.
  Acceptably, less than 35 lines.

there are varying opinions. I try to keep methods under 10 lines if possible.

With the method createPayment(), the lines that handle creating the payment and CreditOrder could be moved out to a separate method:

/**
* Create a payment record and a CreditOrder
* @param $bundle
* @return Payment
*/
private function _createPaymentAndOrder($bundle)
{
    $order_id = time();
    $payment = $this->payment->create([
        "amount"            => $bundle['price'],
        "description"       => $bundle['credits'] . " credits",
        "redirectUrl"       => route('order.credits.finish', ['order_id' => $order_id]),
        "webhookUrl"      => route('order.credits.webhook'),
        "metadata"          => ['order' => 'credits', 'order_id' => $order_id],
    ]);

    CreditOrder::create([
        'user_id' => auth()->user()->id,
        'order_id' => $order_id,
        'payment_id' => $payment->id,
        'payment_status' => $payment->status,
        'credits' => $bundle['credits'],
    ]);
    return $payment
}

Then those lines can be moved out and replaced with a call to that new method:

/**
* Attempt to create a payment record
* @param Request $request
* @param $bundle
*/
public function createPayment(Request $request, $bundle)
{
    $bundles = $this->creditBundles();

    if (!array_key_exists($bundle, $bundles)) {
        return redirect()->route('order.credits');
    }

    $bundle = $bundles[$bundle];
    $payment = $this->_createPaymentAndOrder($bundle);

    return redirect($payment->getPaymentUrl());
}

The names could be modified slightly so they aren't so close.

As far as the finish() method goes, the three lines below that actually update data could be moved into a separate method, unless that feels too small.

$user->increment('credits', $order->credits);
$order->payout = true;
$order->save();

And one could move the first couple blocks that check that the user is the same as the user on the order and the payment is paid into a separate method to handle error checking. If that method returned a certain value (e.g. true to signify an error occurred, false that no error occurred) then the rest of the method could be run accordingly.


Implementation of the Strategy pattern I am learning the Strategy pattern but hate the idea of passing a new object to the Context each time I need to change the desired algorithm. Instead I think it would be best to create an enum that holds all of the Concrete Strategy objects, and simply update them with a setter on the context. Below is my implementation. Please let me know if this is a good approach for Strategy?

Client

public class Client {

    public static void main(String args[]){

        IntroductionContext introductionContext = new IntroductionContext();
        introductionContext.setStrategy(ConcreteStrategies.HELLO);
        introductionContext.executeStrategy();
        introductionContext.setStrategy(ConcreteStrategies.GOODBYE);
        introductionContext.executeStrategy();
    }
}

Context

public class IntroductionContext {

    private IInteractionStrategy iInteractionStrategy;

    public void executeStrategy() {
        if(iInteractionStrategy == null){
            throw new RuntimeException("Context must set its strategy before invoking executeStrategy()");
        }
        this.iInteractionStrategy.interact();
    }

    public void setStrategy(ConcreteStrategies concreteStrategy){
        this.iInteractionStrategy = (IInteractionStrategy) concreteStrategy.getConcreteStrategy();
    }
}

Enum to hold all strategy objects

public enum ConcreteStrategies {

    HELLO(new HelloStrategy()), GOODBYE(new GoodByeStrategy());

    private Object concreteStrategy;

    private ConcreteStrategies(Object concreteStrategy){
        this.concreteStrategy = concreteStrategy;
    }

    public Object getConcreteStrategy(){
        return this.concreteStrategy;
    }
}

Strategy (Interface)

public interface IInteractionStrategy {

    public void interact();
}

Concrete Strategy

public class HelloStrategy implements IInteractionStrategy {

    @Override
    public void interact() {
        System.out.println("Hello!");
    }
}

Concrete Strategy

public class GoodByeStrategy implements IInteractionStrategy {

    @Override
    public void interact() {
        System.out.println("Goodbye!");
    }
}

The overall quality of your code is very good. It is, at least in my opinion, perfectly fine to have an enum containing the strategy implementations.

A few comments and suggestions though:

private Object concreteStrategy; in your enum can and should be private IInteractionStrategy concreteStrategy; instead. No need to use Object references at all in your enum when you can use IInteractionStrategy instead. By doing this, you won't have to typecast in your setStrategy method.
After implementing the above, private IInteractionStrategy concreteStrategy can and should be marked final.
Even though it's fine to store all your strategies in your enum, you shouldn't enforce it. By having the method public void setStrategy(ConcreteStrategies concreteStrategy) you deny any strategies that are not part of your enum. Either change this method, or add a new one, to be public void setStrategy(IInteractionStrategy concreteStrategy)
IntroductionContext could include a executeStrategy(IInteractionStrategy) method to execute a specific strategy. Then you don't need to first call setStrategy and then call executeStrategy (This is only a suggestion, use it if you like)


Documentation for a Minecraft server plugin I'm working on a minecraft plugin for my Spigot Minecraft server.

I have a class which works as expected. I've done my best to hammer every single bug out (famous last words, I know) and am now taking a look at the way it's written. I've noticed that documentation definitely is not lacking, and looks over documented, to the point where it becomes unhelpful. My code is as follows:

package <redacted>;

import java.util.List;
import java.util.Random;

import net.md_5.bungee.api.chat.BaseComponent;
import net.md_5.bungee.api.chat.TextComponent;
import net.md_5.bungee.chat.ComponentSerializer;

import org.bukkit.entity.Player;
import org.bukkit.scheduler.BukkitRunnable;

import org.inventivetalent.bossbar.BossBar;
import org.inventivetalent.bossbar.BossBarAPI;

/**
 *
 * @author Spotlight
 */
public class BossBarUpdate extends BukkitRunnable {

    private Player p;

    /**
     * Set global variables.
     *
     * @param p Player
     */
    public BossBarUpdate(Player p) {
        this.p = p;
    }

    @Override
    public void run() {
        // Cancel task if player logged off or crashed
        if (p == null || !p.isOnline()) {
            p = null;
            cancel();
            return;
        }
        if (!Main.SHOW_BAR) {
            // Do nothing, as AttentionManager or another class has taken over.
            return;
        }
        // Get list of possible messages
        List<String> messages = Main.barConfig.getStringList("messages");
        // Create color instance
        BossBarAPI.Color barColor;
        // Get current (first) boss bar
        BossBar firstBossBar = BossBarAPI.getBossBars(p).iterator().next();
        // Create possible string instance
        String possibleMessage;
        // Make current message a String
        BaseComponent[] components = ComponentSerializer.parse(firstBossBar.getMessage());
        String currentMessage = new TextComponent(components).toLegacyText();
        // Choose a random color + message
        do {
            // Get random color
            barColor = new Utils(null).randomEnum(BossBarAPI.Color.class);
            // Get random number between message size
            int index = new Random().nextInt(messages.size());
            // Get random message at random number index
            possibleMessage = messages.get(index);
        } while (!firstBossBar.getColor().equals(barColor) & currentMessage.equals(possibleMessage));

        TextComponent message = new TextComponent(TextComponent.fromLegacyText(possibleMessage));
        // Temporarily reset bars
        BossBarAPI.removeAllBars(p);
        // Choose a random color
        BossBarAPI.addBar(p, // The receiver of the BossBar
                message, // Displayed message
                barColor, // Color of the bar
                BossBarAPI.Style.PROGRESS, // Bar style
                1.0F); // Fullness
    }
}

I'm looking to increase the helpfulness of this code's documentation, so that it's not as verbose, but you can still tell what's going on. All comments are welcome, even if they aren't related to documentation.

First of all, there is a dinstinction between comments and documentation.

Documentation (Javadoc) defines the contract of what it documents. The Javadoc on a class, for example, describes what the purpose of the class is and can even give examples on how to use it. On a method, Javadoc describes what the method should be used for, along with its parameters, return type, potential exceptions... This documentation is not embedded directly in the code, but on top of it, as specification.
Comments are private; they are inside the code itself and clarify how the implementation is working.

If you want an example, you can look at the Javadoc for the HashMap class and you can then peek into the comments inside the implementation. Both describe completely different things, because they target different issues: the documentation specifies the class, the comments explain the implementation.

You are right that most of the comments are unhelpful and mostly useless. The point of a comment is to clarify the intent of the code; that is to say, not repeat it, but explain why it is doing what it is doing.

As an example, you currently have:

// Create color instance
BossBarAPI.Color barColor;
// Get current (first) boss bar
BossBar firstBossBar = BossBarAPI.getBossBars(p).iterator().next();

The first comment is, in fact, misleading as it says that the code is creating a color instance, when, in fact, it is not. It is just declaring a new variable barColor without initializing it.
The second comment is just a repeat of the code. If there were something to comment here, it would not be that we're getting the first boss bar (which the code iterator().next() already conveys sufficiently clearly); it would explain why we're getting the first boss bar to begin with.

Another example is:

BossBarAPI.addBar(p, // The receiver of the BossBar
        message, // Displayed message
        barColor, // Color of the bar
        BossBarAPI.Style.PROGRESS, // Bar style
        1.0F); // Fullness

Having a comment on every parameter is too much. This should normally be documented properly with Javadoc on the addBar method, which would explain what each parameter is. An IDE can then simply show this Javadoc (on hover for example), which will actually give more information that those short comments.

However, there are useful ones:

if (!Main.SHOW_BAR) {
    // Do nothing, as AttentionManager or another class has taken over.
    return;
}

Arguably, it isn't directly obvious that doing nothing at that place was the intention. The comment clearly explains why it is the case and helps future developers: they won't need to scratch their head wondering why this code is there.

That said, you are really missing the most important documentation of all, which is the Javadoc.

The class BossBarUpdate doesn't have any.
The constructor of BossBarUpdate "Set global variables", without mentioning what those variables are, what they are used for or what the single parameter Player p is for.
The public method run() is also without documentation. How are users of that class supposed to know what it does?


Project Euler #20 solution in Clojure Problem:

  \$n!\$ means \$n × (n − 1) × ... × 3 × 2 × 1\$

  For example, \$10! = 10 × 9 × ... × 3 × 2 × 1 = 3628800\$, and the sum of the digits in the number \$10!\$ is \$3 + 6 + 2 + 8 + 8 + 0 + 0 = 27\$.

  Find the sum of the digits in the number \$100!\$.

My solution in Clojure:

(reduce + (map (fn[x](Integer. (str x))) (seq (str (apply *' (range 1 101))))))

Questions:

Is there a way to avoid the *' in the factorial bit? (apply *' (range 1 101))
I converted the result of the factorial to a string, then to a sequence, and then mapped an Integer cast to a string cast.  Surely there must be a way to simplify this?

Your first question: you could make range return a list of bigints, and reduce over it

(reduce * (range (bigint 1) 101))

your second question: :

you dont have to explicitly use seq, clojure will automatically treat your string as a seq
you dont have to use the full-blown string to number converter, you could for example use int to get the char code:

(map #(- (int %) (int \0)) "1234")`

for other ways of getting digits of a number, check out this thread


Prompting for five test scores and printing the average I am not sure how to properly make this readable. I have been trying to find examples online but I cannot find any that are specific to this simple program.

import java.util.Scanner;

public class KifahUnit3 {

    public static void main(String[] args) {

        Scanner stdIn = new Scanner(System.in);
        String first;  //first name
        String last;  //last name
        String full;  //full name
        Boolean nameMatch = false;
        int score1;
        int score2;
        int score3;
        int score4;
        int score5;
        int totalScore;
        int x;
        double scoreAverage;
        System.out.print("Please enter your first name: ");
        first = stdIn.next();
        System.out.print("Please enter your last name: ");
        last = stdIn.next();
        System.out.println(first + " " + last);
        nameMatch = first.equals(last);
        if (nameMatch == true) {
            System.out.println(
                    "Hello, " + first + " " + last + " "
                    + "your first and last name are the same.");
        } else {
            System.out.println(
                    "Hello, " + first + " " + last + " "
                    + "your first and last name are different.");
            full = first + last;
            System.out.println("The length of your first name is: " + first.length());
            System.out.println("The length of your full name is: " + full.length());
            System.out.println("Your initials are " + first.charAt(0) + last.charAt(0));

            System.out.println("Please enter your test scores");
            System.out.print("Score 1= ");
            score1 = stdIn.nextInt();
            System.out.print("Score 2= ");
            score2 = stdIn.nextInt();
            System.out.print("Score 3= ");
            score3 = stdIn.nextInt();
            System.out.print("Score 4= ");
            score4 = stdIn.nextInt();
            System.out.print("Score 5= ");
            score5 = stdIn.nextInt();
            scoreAverage = (score1 + score2 + score3 + score4 + score5) / 5;
            System.out.println(
                    "Score 1= " + score1 + "; " + "Score 2= " + score2 + "; " + "Score 3= "
                    + score3 + ";  " + "Score 4= " + score4 + "; " + "Score 5= " + score5);
            totalScore = score1 + score2 + score3 + score4 + score5;
            x = 10;
            System.out.println("Total= " + totalScore);
            System.out.println("Average: " + (scoreAverage + x));
            System.out.println("Ooops! Made a mistake. Your average is:  " + scoreAverage);

        }
    } // end main
} // end class KifahUnit3

Firstly - Java Code Convention
Read the Java code convention, see this cheatsheet
The Java Code convention has a more detailed description -
Why use code conventions (From the link) -

Code conventions are important to programmers for a number of reasons:

80% of the lifetime cost of a piece of software goes to maintenance.
Hardly any software is maintained for its whole life by the original
author.
Code conventions improve the readability of the software,
allowing engineers to understand new code more quickly and thoroughly.

Secondly - Refactoring
Start by using arrays - this will really reduce the amount of code you wrote. For example:
// Declare this constant at the top of the class
private static final int NUMBER_OF_SCORES = 5;

int[] score = new int[NUMBER_OF_SCORES];

System.out.println("Please enter your test scores");

// Read all Scores
for (int i = 0; i < NUMBER_OF_SCORES; i++) {
  score[i] = stdIn.nextInt();
}

With arrays, you can also easily calculate the total score and output each score -
// Total Score
int totalScore = 0;
for (int i = 0; i < NUMBER_OF_SCORES; i++) {
  totalScore += score[i];
}

// Output All the scores
for (int i = 0; i < NUMBER_OF_SCORES; i++) {
  System.out.print("score " + i + " = " + score[i]);
}

You can also introduce a Person class - example. (Take note of the getInitials function)
public class Person {

    private final String name;
    private final String surname;

    public Person(String name, String surname) {
        this.name = name;
        this.surname = surname;
    }

    public String getName() {
        return name;
    }

    public String getSurname() {
        return surname;
    }

    public String getInitials() {
        return "" + name.charAt(0) + surname.charAt(0));
    }
}

With the introduction of a Person class, the main function can transform into the for example. (Also splitting up the main function into smaller parts)
public static final int NUMBER_OF_SCORES = 5;

public static void main(String[] args) {
    Scanner stdIn = new Scanner(System.in);
    System.out.print("Please enter your first name: ");
    // Read First Name
    String firstName = stdIn.next();
    System.out.print("Please enter your last name: ");
    // Read Last Name
    String lastName = stdIn.next();
    System.out.println("Hello " + firstName + " " + lastName);
    if (firstName.equals(lastName)) {
        System.out.println("Your first and last name are the same.");
    } else {
        // Now you know you have a valid person.
        Person person = new Person(firstName, lastName);
        System.out.println("Your first and last name are different.");
        processName(person);
        readAndProcessScores();
    }
}

private static void processName(Person person) {
    String full = person.getName() + person.getSurname();
    System.out.println("The length of your first name is: " + person.getName().length());
    System.out.println("The length of your full name is: " + full.length());
    System.out.println("Your initials are " + person.getInitials());
}

public static void readAndProcessScores() {
    Scanner stdIn = new Scanner(System.in);

    int[] score = new int[NUMBER_OF_SCORES];
    System.out.println("Please enter your test scores");
    // Read all Scores
    for (int i = 0; i < NUMBER_OF_SCORES; i++) {
        System.out.print("Score " + i + " = ");
        score[i] = stdIn.nextInt();
        System.out.println();
    }

    // Output All the scores
    for (int i = 0; i < NUMBER_OF_SCORES; i++) {
        System.out.print("score " + i + " = " +score[i] + " ");
    }

    // Total Score
    int totalScore = 0;
    for (int i = 0; i < NUMBER_OF_SCORES; i++) {
        totalScore += score[i];
    }

    double scoreAverage = (((double) totalScore) / NUMBER_OF_SCORES);
    System.out.println("Total = " + totalScore);
    System.out.println("Your average is:  " + scoreAverage);
}

This code itself can be refactored even more, but this I leave to you :)
Try doing the following -

Introduce one more class.

Extract the calculate total score into its own function. Use this declaration:
  public static double calculateAverage(int[] scores)

Add some comments to the methods :)

General tips

Always use good descriptive variable names.
Declare variables as needed (Close to where they are used).

Hope this helps!


A simple Hangman game in Python I'm a noobie in programming so I've made a simple hangman game in python and I'd like someone to tell me how I could improve the code.

edit:How could I do it using classes?

from sys import exit

def makeGallow(gallow):
        for i in range(8):
            gallow.append([])
        gallow[0].append(6 * '_')
        gallow[1].append(' |   |')
        for i in range(2, 7):
            gallow[i].append('     |')
        gallow[7].append(11 * '-')

gallow = list()
secret_word = raw_input("Choose a word: ").lower()
secret_wordls = list()
letters = len(secret_word)
for c in secret_word:
    secret_wordls.append(c)
blanks = list()
for i in range(len(secret_word)):
    blanks.append('_')
lettersUsed = list()
tries = 0
makeGallow(gallow)

while True:

    for i in gallow:
        print "".join(i)
    print "".join(i for i in blanks)
    used = ", ".join(i for i in lettersUsed)
    print "Letters used: " + used
    letter = raw_input("Type a letter: ").lower()
    if letter == 'quit':
        exit(1)
    elif letter in secret_word:
        letters -= 1
        if letters == 0:
            print "You won!"
            exit("Bye!")
        else:
            switch = secret_wordls.index(letter)
            blanks[switch] = letter
    elif letter not in secret_word:
        print "Wrong!"
        tries += 1
        lettersUsed.append(letter)
        if tries == 1:
            gallow[2] = ' O   |'
        elif tries == 2:
            gallow[3] = ' |   |'
        elif tries == 3:
            gallow[3] = '/|   |'
        elif tries == 4:
            gallow[3] = '/|\  |'
        elif tries == 5:
            gallow[4] = ' |   |'
        elif tries == 6:
            gallow[5] = '/    |'
        elif tries == 7:
            gallow[5] = '/ \  |'
            print "Sorry you lost :(."
            print "The word was: " + secret_word
            exit("Bye!")
    elif letter in lettersUsed:
        print "You have already typed that letter!"
        print "Try another one."

edit2: I found a bug where if the work had 2, 3... times the same letter it would count as one and you couldn't win. I change the while loop:

while True:

    for i in gallow:
        print "".join(i)
    print "".join(i for i in blanks)
    used = ", ".join(i for i in lettersUsed)
    print "Letters used: " + used
    letter = raw_input("Type a letter: ").lower()
    if letter == 'quit':
        exit("Bye")
    elif letter in secret_word:
        for i in range(secret_wordls.count(letter)):
            switch = secret_wordls.index(letter)
            blanks[switch] = letter
            secret_wordls[switch] = '-'
            letters -= 1
    elif letter not in secret_word:
        print "Wrong!"
        tries += 1
        lettersUsed.append(letter)
        if tries == 1:
            gallow[2] = ' O   |'
        elif tries == 2:
            gallow[3] = ' |   |'
        elif tries == 3:
            gallow[3] = '/|   |'
        elif tries == 4:
            gallow[3] = '/|\  |'
        elif tries == 5:
            gallow[4] = ' |   |'
        elif tries == 6:
            gallow[5] = '/    |'
        elif tries == 7:
            gallow[5] = '/ \  |'
            print "Sorry you lost :(."
            print "The word was: " + secret_word
            exit("Bye!")
    elif letter in lettersUsed:
        print "You have already typed that letter!"
        print "Try another one."
    if letters == 0:
        print "You won!"
        exit("Bye!")

Whenever I start a review, I open up PEP 8, the Python style guide.  That document is so rarely read that it is depressing.  Your naming is not in accordance with it.  From Method Names and Instance Variables:

  Use the function naming rules: lowercase with words separated by underscores as necessary to improve readability.

(There's more, but it isn't pertinent here)

Your indentation looks good mostly, but for some reason you did eight-space indentation in makeGallow().

In makeGallow(), you do a bunch of .append() calls, but you don't guarantee that your list was empty in the first place.  It is very easy to clear a list.  Just do gallow[:] = [].  As long as you are doing something like that any way, you might as well do gallow[:] = [[] for _ in range(8)] and take out your first for loop.  I used _ as my variable name instead of i because it is the convention in Python to name a variable _ when you aren't using it.

Your code could benefit from an if __name__ == '__main__': block.  That way, you might later create a very fancy hangman game that imports this one to use its makeGallow function.  As it is, importing this would start the game.  I would suggest that you define a main function and call it in your if __name__ == "__main__": block.

Strings are iterable.  That can be useful when making a list of each character in it.  For example, list("foo") is ["f", "o", "o"].  Therefore, you can change

secret_wordls = list()
for c in secret_word:
    secret_wordls.append(c)

to

secret_wordls = list(secret_word)

and change

blanks = list()
for i in range(len(secret_word)):
    blanks.append('_')

to

blanks = list("_" * letters)

Notice that I also changed len(secret_word) to letters.  You defined the variable; why not use it?  While I'm there, is letters really a good name?  From the name, I would expect it to be a list of characters or a string, not a number.  Maybe word_length would be better?

I see that you are using generator expressions.  That's good, but actually a list comprehension is faster when using str.join().  Even better, don't bother with either.

"".join(i for i in blanks)

could be

"". join(blanks)

for example.

I suggest that you add an elif len(letter) != 1 between if letter == "quit": and the rest of the elifs.  That way, you can inform the user that he should type just one letter instead of penalizing him because what he typed isn't in your list of letters.

Th-th-th-th-that's all, folks! (Don't worry; it's a quotation.)


Dynamic thread pool in C++ I have made a thread pool which will dynamically create threads based on how many you need.

#pragma once

#include <thread>
#include <atomic>
#include <mutex>
#include <vector>
#include <functional>
#include <condition_variable>

class thread_pool {
public:
    thread_pool() : stop(false), busy_workers(0) { }

    ~thread_pool() {
        stop = true;
        task_available.notify_all();
        for (auto& worker : workers) {
            if (worker.joinable()) {
                worker.join();
            }
        }
    }

    void run_task(std::function<void()> task) {
        {
            std::lock_guard<std::mutex> task_lock{ task_mu };
            current_task = std::move(task);
        }

        {
            std::lock_guard<std::mutex> workers_lock{ workers_mu };
            if (workers.size() == busy_workers++) {
                workers.emplace_back(work);
                return;
            }
        }

        task_available.notify_one();
    }
private:
    std::atomic_bool stop;
    std::atomic_size_t busy_workers;
    std::vector<std::thread> workers;
    std::function<void()> current_task;
    std::condition_variable task_available;
    std::mutex task_mu;
    std::mutex workers_mu;

    std::function<void()> work = [&]() {
        while (true) {
            std::unique_lock<std::mutex> task_lock{ task_mu };
            task_available.wait(task_lock, [&]() { return current_task || stop; });

            if (!current_task && stop) return;

            auto task = std::move(current_task);
            task_lock.unlock();

            task();
            busy_workers--;
        }
    };
};

The only problem I have with it is the threads will be destroyed before all the tasks have been completed, even though I join all the threads in the destructor.

Bugs

You have several race conditions:

There is no guarantee that a worker stays joinable between these two calls.

        if (worker.joinable()) {
            worker.join();

This is a multi-threaded environment you can not assume run_task is being called from only one thread. You must assume that it can be called from any number of threads.

    {
        std::lock_guard<std::mutex> task_lock{ task_mu };
        // There is no guarantee that `current_task` stays the same
        // between here and when you create a new worker thread.
        // As soon as the scope is exited another thread can enter
        // and overwrite `current_task` before the first thread
        // pushes `work` onto the thread queue or an existing worker
        // picks it up
        current_task = std::move(task);
    }

Design

Usually thread pools are created with a fixed number of threads. Jobs are added to a queue. When a thread becomes available it pulls a job from the queue or waits on a condition variable until a job is available.

I dislike the design of adding threads. As this could spawn lots of threads when there is a sudden high workload that are never released. Threads are expensive resources to create. Also creating many threads does not mean more parallelism (it just means more swapping). A machine usually has an upper bound of available parallelism allocating more threads than this is usually counter productive).


Better way to average around points I have a chunk of code that averages around the points of a grid of values and Im trying to figure out if there is a better way to do the averaging. The array itself is 1 dimensional but i am using an x and y and stride to keep track of it in 2d space. Im mainly just trying to figure out if there is a more optimized way to average this or if this is the best way.

Let me know if anything needs clearing up. Thanks for any input!

for (int y=0;y<lockObj.m_iSize[1];y++)
    {
        for (int x=0;x<lockObj.m_iSize[0];x++)
        {
            if(m_bShouldSmooth)
            {

                float avgAround = 0.0f;
                if(x == 0)
                {
                    if(y==0)
                        avgAround = (pDest[y*lockObj.m_iStride+x] + pDest[y*lockObj.m_iStride+(x+1)] + pDest[(y+1)*lockObj.m_iStride+(x+1)] + pDest[(y+1)*lockObj.m_iStride+x]) / 4;
                    else if(y==lockObj.m_iSize[1]-1)
                        avgAround = (pDest[y*lockObj.m_iStride+x] + pDest[y*lockObj.m_iStride+(x+1)] + pDest[(y-1)*lockObj.m_iStride+(x)] + pDest[(y-1)*lockObj.m_iStride+(x+1)]) / 4;
                    else
                        avgAround = (pDest[(y)*lockObj.m_iStride+(x)] + pDest[(y-1)*lockObj.m_iStride+(x)] + pDest[(y-1)*lockObj.m_iStride+(x+1)] + pDest[(y)*lockObj.m_iStride+(x+1)] + pDest[(y+1)*lockObj.m_iStride+(x+1)] + pDest[(y+1)*lockObj.m_iStride+(x)]) / 6;
                }
                else if(x==lockObj.m_iSize[0]-1)
                {
                    if(y==0)
                        avgAround = (pDest[y*lockObj.m_iStride+x] + pDest[y*lockObj.m_iStride+(x-1)] + pDest[(y+1)*lockObj.m_iStride+(x-1)] + pDest[(y+1)*lockObj.m_iStride+x]) / 4;
                    else if(y==lockObj.m_iSize[1]-1)
                        avgAround = (pDest[y*lockObj.m_iStride+x] + pDest[y*lockObj.m_iStride+(x-1)] + pDest[(y-1)*lockObj.m_iStride+(x-1)] + pDest[(y-1)*lockObj.m_iStride+(x)]) / 4;
                    else
                        avgAround = (pDest[(y)*lockObj.m_iStride+(x)] + pDest[(y-1)*lockObj.m_iStride+(x)] + pDest[(y-1)*lockObj.m_iStride+(x-1)] + pDest[(y)*lockObj.m_iStride+(x-1)] + pDest[(y+1)*lockObj.m_iStride+(x-1)] + pDest[(y+1)*lockObj.m_iStride+(x)]) / 6;
                }
                else
                {
                    if(y==0)
                        avgAround = (pDest[y*lockObj.m_iStride+x] + pDest[y*lockObj.m_iStride+(x-1)] + pDest[(y+1)*lockObj.m_iStride+(x-1)] + pDest[(y+1)*lockObj.m_iStride+x] + pDest[(y+1)*lockObj.m_iStride+(x+1)] + pDest[(y)*lockObj.m_iStride+(x+1)]) / 6;
                    else if(y==lockObj.m_iSize[1]-1)
                        avgAround = (pDest[y*lockObj.m_iStride+x] + pDest[y*lockObj.m_iStride+(x-1)] + pDest[(y-1)*lockObj.m_iStride+(x-1)] + pDest[(y-1)*lockObj.m_iStride+x] + pDest[(y-1)*lockObj.m_iStride+(x+1)] + pDest[(y)*lockObj.m_iStride+(x+1)]) / 6;
                    else
                        avgAround = (pDest[y*lockObj.m_iStride+x] + pDest[y*lockObj.m_iStride+(x-1)] + pDest[(y-1)*lockObj.m_iStride+(x-1)] + pDest[(y-1)*lockObj.m_iStride+x] + pDest[(y-1)*lockObj.m_iStride+(x+1)] + pDest[(y)*lockObj.m_iStride+(x+1)] + pDest[(y+1)*lockObj.m_iStride+(x+1)] + pDest[(y+1)*lockObj.m_iStride+(x)] + pDest[(y+1)*lockObj.m_iStride+(x-1)]) / 9;
                }

                if(this->GetContactForce() > 0){
                    if(avgAround < pDest[y*lockObj.m_iStride+x])
                        pDest[y*lockObj.m_iStride+x] = avgAround;
                }
            }
        }
    }

I'm not into C++, but here's a suggestion for what it is worth.  Create a class that encapsulates the array, let's call it Array. Define an operator[] method that gives you the value of the element of the array at (x,y);  the function returns 0.0 if the coordinates are invalid.  Another method, n_points, returns the number of valid points around (x,y) - ie. 4, 6 or 9. Then you can compute the average using the following function, ignoring where the point (x,y) is located in the array:

float average(const Array &array, int x, int y)
{
    return
        (array[x-1,y-1] + array[x,y-1] + array[x+1,y-1] +
         array[x-1,y]   + array[x,y]   + array[x+1,y] +
         array[x-1,y+1] + array[x,y+1] + array[x+1,y+1])
        / array.n_points(x,y);
}

That won't be more efficient (the reverse probably), but it would be more readable.


A simple BSON indenter / compacter BSON is an extended JSON format. For example these documents are valid BSON documents, but invalid JSON documents:

prettified documents:

[
  {
    "_id": ObjectId("5a934e000102030405000000"),
    "k": 10,
    "dt": ISODate("2018-10-10")
  },
  {
    "_id": ObjectId("5a934e000102030405000001"),
    "k": {
      "field1": "someVal",
      "field2": 12.2323242
    }
  },
  {
    _id: ObjectId("5a934e000102030405000002"),
    k: 7
  }
]

compacted documents

[{"_id":ObjectId("5a934e000102030405000000"),"k":10,"dt":ISODate("2018-10-10")},{"_id":ObjectId("5a934e000102030405000001"),"k":{"field1":"someVal","field2":12.2323242}},{_id:ObjectId("5a934e000102030405000002"),k:7}]

I recently needed a tool to compact or prettify BSON documents. After several attempts with javascript formatter (beautifyjs, prettifier ...) I ended up implementing my own solution.

Important:

the goal of this code is not to check if a document has a valid BSON syntax, but only to prettify/compact a BSON document. Hence, it assumes that the src is valid BSON

Here is my solution:

const compactMode = 0
const indentMode = 1

function indent(src) {
    return write(src, indentMode)
}

function compact(src) {
    return write(src, compactMode)
}

function write(src, mode) {

    var result = ""
    var needIndent = false
    var inValue = false
    var depth = 0

    for (var i = 0; i < src.length; i++) {
        c = src.charAt(i)

        if (c === " " && !inValue || c === "\n") {
            continue
        }

        if (needIndent && c !== "]" && c !== "}") {
            needIndent = false
            depth++
            result += newline(mode, depth)
        }

        switch (c) {
            case "{":
            case "[":
                needIndent = true
                result += c
                break
            case ",":
                result += c
                result += newline(mode, depth)
                break
            case ":":
                result += c
                if (mode === indentMode && !inValue) {
                    result += " "
                }
                break
            case "}":
            case "]":
                if (needIndent) {
                    needIndent = false
                } else {
                    depth--
                    result += newline(mode, depth)
                }
                result += c
                break
            case "\"":
            case "'": // if we get a `'`, replace it with `"`
                inValue = !inValue
                result += "\""
                break
            default:
                result += c
        }
    }
    return result
}

function newline(mode, depth) {
    if (mode === compactMode) {
        return ""
    }
    var line = "\n"
    for (var i = 0; i < depth; i++) {
        line += "  "
    }
    return line
}

How can I improve this ?

the goal of this code is not to check if a document has a valid BSON syntax

But that's the first requirement of a formatter, understanding the syntax. For any formatter to work, it must understand the structure of the input. For instance, if I stick a { inside some random string, how would case "{": tell it apart from a structural {?

So in some form, your parser, no matter how simple, would need to know about some of these rules. You don't need to build a robust parser, you just need to know where the current character is based characters already encountered.

You can use JSON's flow diagrams as a guide to know when it's possible to encounter such characters. You just need to extend it for BSON. Also, here's a simple JS comment remover which removes comments from JS without having to understand the entire JS syntax. In a gist, it went through the string, checking the current character against the current state (i.e. am I in a string, a regexp, comment, etc.) then modifying the code accordingly as it went through.


Eliminating unwanted namespaces I am trying to eliminate unwanted namespaces, and I know there's a better way to do this.  This code works, but it's remedial and redundant.

string sPattern = "xmlns:d5p1=\"http://www.w3.org/2001/XMLSchema-instance\" d5p1:nil=\"true\"";
string sPattern2 = "d5p1:nil=\"true\" xmlns:d5p1=\"http://www.w3.org/2001/XMLSchema-instance\"";
string sPattern3 = "xmlns:d2p1=\"http://www.w3.org/2001/XMLSchema-instance\" d2p1:nil=\"true\"";

string[] fString = new string[sentences.Count()];
//foreach (string str in sentences)
for(int i=0; i < sentences.Count(); i++)
{
    Console.WriteLine("{0,24}", sentences[i]); // lines 0-24.
    if(System.Text.RegularExpressions.Regex.IsMatch(sentences[i], sPattern, System.Text.RegularExpressions.RegexOptions.IgnoreCase))
    {
        Console.WriteLine(" (match for '{0}' found)", sPattern);
        eString = sentences[i].Replace(sPattern, "");

    }
    else if(System.Text.RegularExpressions.Regex.IsMatch(sentences[i], sPattern2, System.Text.RegularExpressions.RegexOptions.IgnoreCase))
    {
        Console.WriteLine(" (match for '{0}' found)", sPattern2);
        eString = sentences[i].Replace(sPattern2, "");
    }
    else if (System.Text.RegularExpressions.Regex.IsMatch(sentences[i], sPattern3, System.Text.RegularExpressions.RegexOptions.IgnoreCase))
    {
        Console.WriteLine(" (match for '{0}' found)", sPattern3);
        eString = sentences[i].Replace(sPattern3, "");
    }
    else
    {
        Console.WriteLine();
        eString = sentences[i];
    }

    fString[i] = eString;
}

// ok, let's look at the end result
foreach(string s in fString)
{

    Console.WriteLine(s);
}

// stop the app from shutting down before seeing results
Console.ReadLine();

Well, the first thing I'd do is put the patterns in a list, and loop through matching them. The second thing would be general cleanup; use namespaces where you can, one-liner if/else clauses don't need braces, use var where the type is obvious, etc etc. This gets rid of that if-elseif structure and makes the code much more concise.

As far as merging the patterns, you could match on a combination of two:

"xmlns:(d5p1|d2p1)=\".*?\"" (we don't care what the exact namespace definition is, just don't match greedily)
"(d5p1|d2p1):nil=\"true\""

If either of these patterns match, replace the matching text with an empty string:

    List<string> patterns = new List<string>{
       "xmlns:(d5p1|d2p1)=\".*?\"",
       "(d5p1|d2p1):nil=\"true\"",
    };

    var output = new string[sentences.Count()];
    for(int i=0; i < sentences.Count(); i++)
    {
        Console.WriteLine("{0,24}", sentences[i]); // 24 chars, left-justified.
        var formattedSentence = sentences[i];

        foreach(var pattern in patterns)
        {
           var len = formattedSentence.Length;
           formattedSentence = Regex.Replace(formattedSentence, pattern, String.Empty,
                 RegexOptions.IgnoreCase)

           if(formattedSentence.Length != len)
              Console.WriteLine(" (match for '{0}' found)", pattern);
        }

        //Finally, this may have left some unneeded whitespace which we can consolidate.
        formattedSentence = Regex.Replace(formattedSentence, "  +", " ");

        output[i] = formattedSentence;
    }

    // ok, let's look at the end result
    foreach(var sentence in output)
       Console.WriteLine(sentence);

One last thing. Hungarian notation - putting some type-specific flag at the beginning of your variable names - is EVIL in virtually any language used in an IDE, including .NET languages. IntelliSense tells you all you need to know about an object's type, if it happens to not be obvious via context; you shouldn't put it in the variable name. If you do, and the type changes, the variable name is now inconsistent and every usage of it needs to change (a task made easier by refactoring assistants like ReSharper, but if you have that you have absolutely no need for Hungarian).


Generic method to split provided collection into smaller collections First time writing a generic method here.  Input a List<T> and an int value and output a List<List<T>> with each member List<T> of the provided int input.

No doubt there are easier / less verbose ways of handling this via LINQ but I wanted to try my hand with a generic method.  Please let me know of any ways that I could improve this method.

static List<List<T>> SplitIntoChunks<T>(List<T> fullBatch, int chunkSize)
{
    if (chunkSize <= 0)
    {
        throw new ArgumentOutOfRangeException("Chunk size cannot be less than or equal to zero.");
    }

    if (fullBatch == null)
    {
        throw new ArgumentNullException("Input to be split cannot be null.");
    }

    int numOfChunks = fullBatch.Count / chunkSize;

    //handles uneven number of items within the full batch to ensure none at the end are missed
    if (fullBatch.Count % chunkSize > 0)
    {
        numOfChunks++;
    }

    int cellCounter = 0;

    List<List<T>> splitChunks = new List<List<T>>();

    for (int chunkNum = 0; chunkNum < numOfChunks; chunkNum++)
    {
        var chunk = new List<T>();
        for (int index = 0; index < chunkSize; index++)
        {
            if (index < fullBatch.Count)
            {
                chunk.Add(fullBatch[index]);
                cellCounter++;
            }
        }
        splitChunks.Add(chunk);
    }

    return splitChunks;
}

Your implementation is not bad for a no-LinQ solution. But there's always room for improvement. First I'll provide a LinQ solution that provides a clean way to return a chunked list:

public static List<List<T>> Split<T>(List<T> collection, int size)
{
    var chunks = new List<List<T>>();
    var chunkCount = collection.Count() / size;

    if (collection.Count % size > 0)
        chunkCount++;

    for (var i = 0; i < chunkCount; i++)
        chunks.Add(collection.Skip(i * size).Take(size).ToList());

    return chunks;
}

Basically it comes down to this:

calculate the count of chunks that are needed
loop over the length of chunks
use the Enumerable.Skip and Enumerable.Take methods to get chunks
return the list of chunks

Now, you implemented a no-LinQ solution so I created one myself too. My implementation doesn't have to calculate the amount of chunks or use two loops to create the list of chunks:

public static List<List<T>> SplitNoLinq<T>(List<T> collection, int size)
{
    var chunks = new List<List<T>>();
    var count = 0;
    var temp = new List<T>();

    foreach (var element in collection)
    {
        if (count++ == size)
        {
            chunks.Add(temp);
            temp = new List<T>();
            count = 1;
        }
        temp.Add(element);
    }

    chunks.Add(temp);
    return chunks;
}

The code iterates over the collection and keeps a counter, adding the iterated item to a temporary list. If the counter equals the desired length of a chunk it will add the temporary list to the return list. At the end, the last chunk is added.

The var keyword:

From the C# Programming Guide:

  The var keyword can also be useful when the specific type of the variable is tedious to type on the keyboard, or is obvious, or does not add to the readability of the code.

So lines like:

List<List<T>> splitChunks = new List<List<T>>();

would become:

var splitChunks = new List<List<T>>();

Furthermore you could place the code in an extension method, also using an IEnumerable<T> instead of List<T>:

public static class Extensions
{
    public static List<List<T>> Split<T>(this IEnumerable<T> collection, int size)
    {
        var chunks = new List<List<T>>();
        var count = 0;
        var temp = new List<T>();

        foreach (var element in collection)
        {
            if (count++ == size)
            {
                chunks.Add(temp);
                temp = new List<T>();
                count = 1;
            }
            temp.Add(element);
        }
        chunks.Add(temp);

        return chunks;
    }
}

//USAGE::
var numbers = new List<int> { 1, 2, 3, 4, 5, 6, 7, 8, 9, 0 };
var chunked = numbers.Split(5);


Improvements with caesar cipher encryption I have wanna learn more of ways to improve my code and also, make it look different. I want to see different approach but have the same result.

def text():
    print('What message(s) are you trying to encrypt?')
    return input()

def shift():
    key = 0;
    while True:
        print('Enter the key number (1-%s)' % (keymax))
        key = int(input())
        if (key >= 1 and key <= keymax):
            return key

def encrypt(string, shift):
    hidden = ''
    for char in string:
        if char.isalpha() == False:
            hidden = hidden + char
        elif  char.isupper():
            hidden = hidden + chr((ord(char) + shift - 65) % 26 + 65)
        else:
            hidden = hidden + chr((ord(char) + shift - 97) % 26 + 97)

    return hidden

text = text()
s = shift()

print("original string: ", text)
print("after encryption: ", encrypt(text, s))

It would be gladly appreciated if anyone could lend a hand or help out! :)

Just a couple of small things you could improve/change on your code:

Unnecessary function/print statement. You can just do text = input('What message ... encrypt?\n') instead of making an extra function for it an using a seperate print statement.
The same goes for the input statement in the shift function, you can put the text of the print statement inside of the input.
Unnecessary default value for variable.
In the shift function you assign the value 0 to the variable key on forehand. But as you never use it outside the while True statement this is unnecessary. This line can be removed.
'Magic numbers'. Inside the encrypt function you use the ascii codes 65, 26 and 97 to quickly convert the characters. While their meaning might be obvious to you (and in this trivial case to most people) it is never a good idea to do this. It is cleaner to make a variable on top of your program in uppercase defining the value. So something like: UPPER_ASCII_SHIFT = 65 and then using that variable in the rest of the code. This way it is much easier to read for someone else. This is also a big theme in the cs50 course where this programming problem is originally from.
Try choosing clearer names for variables that immediately clarify their use and content. Something like hidden is a bit ambiguous, maybe encrypted_string is better? Also try to avoid single letter variable names like s, but rather call it something like shift_distance.


Rust dynamic programming : word break I've solved this problem: https://leetcode.com/problems/word-break

  Given a non-empty string s and a dictionary wordDict containing a list
  of non-empty words, determine if s can be segmented into a
  space-separated sequence of one or more dictionary words.

  Note:

  The same word in the dictionary may be reused multiple times in the
  segmentation. You may assume the dictionary does not contain duplicate
  words. Example 1:

impl Solution {
    fn dfs(i: usize, words: &HashSet<String>, cur_word: &str, cache: &mut HashMap<usize, bool>, n: usize, orig:&str) -> bool {

        if i == n {
            return true;
        }
        if cache.contains_key(&i) {
            return *cache.get(&i).unwrap();
        }
        if words.contains(cur_word) {
            cache.insert(i, true);
            return true;
        }
        let mut res = String::from("");
        for k in i..n {
            let s = orig.chars().nth(k).unwrap();
            res.push(s);
            if words.contains(&res) && Self::dfs(k + 1, &words, orig.get(k+ 1..).unwrap_or_default(), cache, n, orig) {
                cache.insert(i, true);
                return true;
            }
        }
        cache.insert(i, false);
        return false;
    }
    pub fn word_break(s: String, word_dict: Vec<String>) -> bool {
        let set = word_dict.into_iter().collect::<HashSet<String>>();
        let mut cache = HashMap::new();
        Self::dfs(0, &set, "", &mut cache, s.len(), &s)
    }
}

Please ignore the  useless Solution struct -- that's part of the website's interface for solution.

I'm quite upset with how my dfs function is structured -- I am passing far too many arguments into it.

In python, I would've simply created a recursive nested function, which would have captured outer variables; rust doesn't allow for recursive closures, as a closure is an unnamed struct. So I've had to resort to creating a argument-heavy function.

How do I make my code more idiomatic?

My main issue with this code is the function interface as you already identified yourself. These are the most glaring issues in my opinion:

Parameter names are not clear. Names like i and n might be good enough in very short, self-contained loops/closures, but not as function parameters. I similarly don't like abbreviations like orig, and cur because the full words are not much longer. Always consider you first see this function, is it original or origin, is it current or cursor? Your interface should be clear without reading the code.
You are always passing a full string slice while you know you will only read from a certain offset (i).
Passing n is redundant, as almost any check that you do against this can be replaced by checking if you reached the end of orig.
Similarly but less obvious passing i is redundant. The obvious part is related to the first point, if you only pass the slice this represents offset 0. Now, you also use this as a cache key, but that's not necessary. If instead of passing the full slice you only push the 'remaining' string you can use this as cache key to exactly the same effect.
You insert true in the cache, which requires it to be a map, but there are several reasons why this is useless. I'd just use a HashSet that is only updated for false. Reasons are as follows:

If you found a 'true', the function finished anyway.
When indexed by usize and with this judging system you can't use the cache for multiple string inputs (and never for multiple word inputs). So the true cache is even useless here.
But even if you could re-use the cache, the true adds little, because you can quickly prune the false branches already.

You explicitly end with return false;, it is more idiomatic to simply say false (without the semicolon).
You have a whole construct that basically loops over a splitted version of your input. This can be significantly simplified by using the built-in split_at.

Based on all this I refactored your original algorithm to the following:

use std::collections::HashSet;
impl Solution {
    fn dfs(remaining: &str, words: &HashSet<String>, mut cache: &mut HashSet<String>) -> bool {
        for i in 1..remaining.len() + 1 {
            let (word, remaining) = remaining.split_at(i);
            if words.contains(word.into())
                && !cache.contains(remaining.into())
                && Self::dfs(remaining, &words, &mut cache)
            {
                return true;
            }
        }
        if remaining.len() == 0 {
            true
        } else {
            cache.insert(remaining.into());
            false
        }
    }
    pub fn word_break(s: String, word_dict: Vec<String>) -> bool {
        let set = word_dict.into_iter().collect::<HashSet<String>>();
        let mut cache = HashSet::new();
        Self::dfs(&s.as_str(), &set, &mut cache)
    }
}

This may not yet be ideal, I for example don't like remaining and dfs as names. Other improvements you could try but are honestly overkill for a braintease exercise:

It seems somewhat weird to pass words and cache as a parameter. More idiomatic would be to make a WordBreakChecker struct. You provide it with a new initializer that sets the cache and words, and then have a single dfs (or better check) function that only takes the string as input.
In my example I still index the slice explicitly by using a for loop and split_at. That's good enough for here, but it may be interesting practice to implement the Iterator trait to iterate over these combinations directly (honestly, it may exist already in std or a crate, I didn't look to hard).


Python implementation of a singly linked list This is my code for the implementation of a singly linked list in Python:

class SList:
    def __init__(self):
        self.root = None
        self.size = 0

    def insert(self, item):
        try:
            if not item:
                raise ValueError
            self.size += 1
            if self.root is None:
                self.root = Node(item)
            else:
                p = Node(item)
                p.next = self.root
                self.root = p
        except ValueError:
            raise ValueError('Cannot add None item to a list')

    def remove(self, index):
        try:
            if index < 0 or index >= self.size:
                raise ValueError

            current = self.root
            count = 0
            while count < index:
                current = current.next
                count += 1
            current.next = current.next.next
            self.size -= 1
        except ValueError:
            raise ValueError('Index cannot be negative or greater than the size of the list')

    def __sizeof__(self):
        return self.size

class Node:
    def __init__(self, data):
        try:
            if not data:
                raise ValueError
            self.data = data
            self.next = None
        except ValueError:
            raise ValueError('Node cannot be instantiated without an item')

raise + except + raise = raise

def remove(self, index):
    try:
        if index < 0 or index >= self.size:
            raise ValueError

            ...

    except ValueError:
        raise ValueError('Index cannot be negative or greater than the size of the list')

It make no sense to except and raise again, simplify to:

if index < 0 or index >= self.size:
    raise IndexError('Index cannot be negative or greater than the size of the list')

...

The function will look like:

def insert(self, item):
    if item is None:
        raise ValueError('Cannot add None item to a list')
    self.size += 1
    if self.root is None:
        self.root = Node(item)
    else:
        p = Node(item)
        p.next = self.root
        self.root = p

for instead of counting loop

for is a more streamlined way of looping n times than a while loop:

        count = 0
        while count < index:
            current = current.next
            count += 1

Becomes:

for _ in range(index):
    current = current.next

(Please note _ is a convention meaning the value will not be used)


Calculating the potential of a matrix of size (424600, 55) I have the following working code to calculate the potential of a matrix after normalizing the matrix. But it is too much time consuming. Is there a way to make this code run faster?

import csv
import numpy as np
from normalization import normalized_matrix
n = len(normalized_matrix)
sigma = 1
potential_writer = csv.writer(open('potential.csv', 'w'))
for l in range(n):
    elem = []
    for j in range(len(normalized_matrix[0])):
        GWF = 0
        PotSecPart = 0
        for i in range(n):
            if (i!=l):
                PotSecPart = (PotSecPart + (((normalized_matrix[l][j] - normalized_matrix[i][j])**2)*(np.exp((-(normalized_matrix[l][j] - normalized_matrix[i][j])**2)/(2*(sigma**2))))))
                GWF = GWF + (np.exp((-(normalized_matrix[l][j] - normalized_matrix[i][j])**2)/(2*(sigma**2))))
                PotFirPart = (2*(sigma**2))* GWF
        elem.append(np.round(PotSecPart/PotFirPart, 2))
    potential_writer.writerows([elem])

Your code loops N x M x N times, where N is currently 424600 and M is 55. I don't think it's going to finish.

I tried a couple of variations of your code, and the results are below. The version marked "original" is pretty true to your original code, except that I returned the results instead of storing them in a file. (So I could compare them against the original version.)

The version marked "fortran" is an attempt to change the internal arrangement of data in the array, to try to get better cache performance out of consecutive reads. It didn't work, but I may not have done it right, or I maybe should have done it on the numpyier version. (It's late, I'm tired, sorry.)

The "cached" version is the original code, but I tried to cache everything I could to eliminate repeated reads of the same items. I also tried to eliminate repeated sub-expressions, and I eliminated the sigma variable, since it was 1 and never changed.

The "numpyier" version uses numpy operations in the inner loop. There may be a way to do the same thing with the columns, doing a 2-d computation and eliminating the middle loop as well. Someone better at numpy than I am may be able to provide that. At any rate, this version does not produce quite the same results, but they're close and it runs a lot faster.

import sys
import csv
from timeit import timeit

import numpy as np
#from normalization import normalized_matrix
#MAKE_ROWS = 424600
MAKE_ROWS = 424
MAKE_COLS = 55
normalized_matrix = np.random.rand(MAKE_ROWS, MAKE_COLS)

def do_original():
    print(f"Original method", file=sys.stderr)
    n = len(normalized_matrix)
    sigma = 1
    #potential_writer = csv.writer(open('pot_orig.csv', 'w'))
    writer = []

    for l in range(n):
        elem = []
        for j in range(len(normalized_matrix[0])):
            GWF = 0
            PotSecPart = 0
            for i in range(n):
                if (i!=l):
                    PotSecPart = (PotSecPart + (((normalized_matrix[l][j] - normalized_matrix[i][j])**2)*(np.exp((-(normalized_matrix[l][j] - normalized_matrix[i][j])**2)/(2*(sigma**2))))))
                    GWF = GWF + (np.exp((-(normalized_matrix[l][j] - normalized_matrix[i][j])**2)/(2*(sigma**2))))
                    PotFirPart = (2*(sigma**2))* GWF
            elem.append(np.round(PotSecPart/PotFirPart, 2))
        #potential_writer.writerows([elem])
        writer.append(elem)
    return writer

def do_cached():
    print(f"Cached method", file=sys.stderr)
    num_rows = len(normalized_matrix)
    num_cols = len(normalized_matrix[0])

    sigma = 1
    #potential_writer = csv.writer(open('pot_orig.csv', 'w'))
    writer = []

    exp = np.exp

    for l in range(num_rows):
        elem = []
        for j in range(num_cols):
            GWF = 0
            PotSecPart = 0

            nm_lj = normalized_matrix[l][j]

            for i in range(num_rows):
                if i != l:
                    nm_ij = normalized_matrix[i][j]
                    diff_sq = (nm_lj - nm_ij) ** 2
                    exp_diff_sq_2 = exp(-diff_sq/2)

                    PotSecPart += diff_sq * exp_diff_sq_2
                    GWF += exp_diff_sq_2
                    PotFirPart = 2 * GWF
            elem.append(np.round(PotSecPart/PotFirPart, 2))
        #potential_writer.writerows([elem])
        writer.append(elem)
    return writer

def do_fortran():
    print(f"Fortran (cached) method", file=sys.stderr)
    matrix = normalized_matrix
    shape = matrix.shape
    matrix = np.reshape(matrix, shape, order='F')
    num_rows, num_cols = shape

    sigma = 1
    #potential_writer = csv.writer(open('pot_orig.csv', 'w'))
    writer = []

    exp = np.exp

    for l in range(num_rows):
        elem = []
        for j in range(num_cols):
            GWF = 0
            PotSecPart = 0

            nm_lj = matrix[l][j]

            for i in range(num_rows):
                if i != l:
                    nm_ij = matrix[i][j]
                    diff_sq = (nm_lj - nm_ij) ** 2
                    exp_diff_sq_2 = exp(-diff_sq/2)

                    PotSecPart += diff_sq * exp_diff_sq_2
                    GWF += exp_diff_sq_2
                    PotFirPart = 2 * GWF
            elem.append(np.round(PotSecPart/PotFirPart, 2))
        #potential_writer.writerows([elem])
        writer.append(elem)
    return writer

def do_numpyier():
    print(f"Numpy-ier method", file=sys.stderr)
    matrix = normalized_matrix
    num_rows, num_cols = normalized_matrix.shape

    sigma = 1
    #potential_writer = csv.writer(open('pot_orig.csv', 'w'))
    writer = []

    exp = np.exp

    for l in range(num_rows):
        elem = []
        for j in range(num_cols):
            GWF = 0
            PotSecPart = 0

            nm_lj = normalized_matrix[l][j]

            diff_sq = (nm_lj - matrix[:, j]) ** 2
            np.delete(diff_sq, l)
            exp_diff_sq_2 = exp(-diff_sq / 2)
            np.delete(exp_diff_sq_2, l)

            PotSecPart = np.sum(diff_sq * exp_diff_sq_2)
            GWF = np.sum(exp_diff_sq_2)
            PotFirPart = 2 * GWF

            elem.append(np.round(PotSecPart/PotFirPart, 2))
        #potential_writer.writerows([elem])
        writer.append(elem)
    return writer

if __name__ == '__main__':
    #orig = do_original()
    #print(timeit('do_original()', globals=globals(), number=1))
    #cached = do_cached()
    #assert cached == orig
    print(timeit('do_cached()', globals=globals(), number=1))
    #fortran = do_fortran()
    #assert fortran == orig
    #print(timeit('do_fortran()', globals=globals(), number=1))
    #numpyier = do_numpyier()
    #assert numpyier == cached
    print(timeit('do_numpyier()', globals=globals(), number=1))

The cached version was a little better than 2x faster than the original version, when I ran it on a 42x55 array. After a few passes, I stopped running the original.

The fortran version surprised me, because I expected to see better performance. Its results were consistently a smidge faster (.01-.02 sec) than the cached version, but that's it.

The numpyier version uses numpy operations to do the individual element calculations. This effectively replaces the inner loop with C.

Please Note: The numpyier version gets different results than your python-only version. The differences are typically +/- 0.01 in your rounded output. But they're different, and I haven't looked into why.

I did most of my work at a shape of 42 x 55. That was in the 3-second range for me, about what I was willing to run repeatedly. After I got the numpyier version sort-of working, and satisfied myself that the differences were small, I ran it again with a shape of 424 x 55. That's 10 times bigger, which means 100x slower since this is an \$n^2\$ operation. The results were:

Cached method
114.699519728
Numpy-ier method
4.552203466999998

This seems intuitively correct: the numpy-ier version eliminates the python inner loop in favor of C looping operations, reducing the runtime by a factor of 424. The results are 23x faster, which is on the order of 100x.

Note that this suggests that increasing the shape by a factor of 1000 (424600 instead of 424) would increase the runtime by somewhere between 1,000x and 1,000,000x. The numpyier version would require somewhere between 4000 seconds (1 hr, 20 mins) and 4,000,000 seconds (50 days).

I am working on a 10+ year old, 32-bit laptop. Your mileage is very likely going to be better.


Mixing function parameters, commands and command arguments in Bash functions I'm trying to write a bash function that will take a command such as xcode-select -v or brew -v that outputs a lot of extra text, and format it to my liking.  Just to further the example, say I run xcode-select -v, it will output xcode-select version 2333..  I want to be able to split this string up and only take the 2333 part so I can put it in an echo statement.  I want the same function to be able to handle the various outputs of stuff like brew -v, or git --version, etc.

get_version_number() {
    # set local variable to executed arguments that is passed in
    local command="$($@)"
    # set up temp variable and assign to global IFS variable
    OIFS=$IFS
    # set up IFS to split the string on empty space
    IFS=" "
    # read commands output into an array and store it in $array variable
    read -a array <<< "$command"
    # clear out IFS back to what it original was for further use
    IFS=$OIFS
    # echo out array at a particular indices that should be passed in
    echo ${array[@]}
}

# store output of this command into variable for further usage
version=$(get_version_number xcode-select -v)
echo $version

Edit 1:
Originally the idea came from me having to copy/paste these same lines within different conditional blocks...

command=$(git -v)
OIFS=$IFS
IFS=" "
read -a output_array <<< "$command"
IFS=$OIFS
printf "Git version ${output_array[2]} is installed."

command=$(brew -v)
OIFS=$IFS
IFS=" "
read -a output_array <<< "$command"
IFS=$OIFS
printf "Homebrew version ${output_array[1]} is installed."

The trick for splitting up the version blurb for just the version number, is to pass the values you want in before the command, and then shift them off $@.
you have done a relatively unfamiliar-to-me parsing mechanism with the resetting the field separators, etc. Using bash as a parser is a problem for me (and I've never hated myself enough to try....).
I would set up a function that takes a co-ordinate of a version as a line/word combination, and rely on out-of-bash tools to do it......
I will probably get nailed for starting 4 sub-processes, but head, tail, and cut are small.... right?  (Forgive me already, you won't be calling this in a tight loop, will you? You're already doing that a bit with the bash $( ...) operator...)
get_version_number() {
    local line=$1
    shift
    local word=$1
    shift

    # echo Line $line Word $word and Commend $@

    # set local variable to executed arguments that is passed in
    echo "$( $@ | head -$line | tail -1 | cut -d ' ' -f $word)"
}

Then you would call it like:
gitver=$(get_version_number 1 3 git --version)
perlver=$(get_version_number 2 4 perl -version)

echo Git and Perl are $gitver and $perlver respectively

which for me, produces:

Git and Perl are 1.7.1 and v5.10.1 respectively

This also allows you to do things like get the redhat version as
rhelver=$(get_version_number 1 7 cat /etc/issue)
javaver=$(get_version_number 1 3 java -version)

and even things like the current time 23:45:08 ;-)
time=$(get_version_number 1 4 date)

Some notes:

historically, it is 'expensive' to start lots of child processes. It still is a problem, but not quite as bad as it was. Doing that hundreds or thousands of times may add up to seconds of time wasted. If you can avoid creating child processes, do it.... but, sometimes it's just easier to be lazy than to be super-bash-smart.
I create an additional 4 processes (actually 5) each time you call the function. One for the $( ... ), one for the $@ inside that, one for head, one for tail, and one for cut. That is almost excessive.... but if you are calling this 100 times, then it's OK, if you are calling it 1000 times, you will be able to save seconds by doing it differently.
the $( ... ) operator is formally called the Command Substitution. What it means is that everything inside the structure is run as if in a 'child' bash process.... (so a new process is created, and the output is substituted in place of the command)
what the shift operator does is take $@ and remove the first item.... so, your function called as get_version_number 1 3 git --version gets 4 parameters, 1, 3, git and --version. We copy $1 off as $line, and then shift $@, so there are now only 3 parameters in $@. We then copy $1 again (but because of the shift, it is a different value) as $word, and then shift again to remove it. We are left with just git and --version as the parameters.

If you wanted, for example, to save one process each time, you could instead do:
get_version_number() {
    local line=$1
    shift
    local word=$1
    shift
    GOTVERSION="$( $@ | head -$line | tail -1 | cut -d ' ' -f $word)"
}

get_version_number 1 3 java -version
javaversion=$GOTVERSION

That saves using the $( .... ) and echo combination, so you save a process, but the code is a bit more complicated.

Edit 2.
'Obviously', what I suggest you do is not necessarily the best thing. Your code was working fine, and the mechanism will be faster (slightly) than mine because it does not do the additional 4 processes and keeps the logic inside bash internal commands.
Here is my 'shift' system applied to your code:
get_version_number() {
    local word=$1
    shift
    # set local variable to executed arguments that is passed in
    local command="$($@)"
    # set up temp variable and assign to global IFS variable
    OIFS=$IFS
    # set up IFS to split the string on empty space
    IFS=" "
    # read commands output into an array and store it in $array variable
    read -a array <<< "$command"
    # clear out IFS back to what it original was for further use
    IFS=$OIFS
    # echo out array at a particular indices that should be passed in
    echo ${array[$word]}
}

and you would call it with the word-number for the version as the first argument:
gitver=$(get_version_number 3 git --version)

But this does not allow you to find versions on multi-line outputs.....


Laravel 5.8: prevent duplicated code Sometimes we have this code blocks that is repeated in the same controller again and again, I reviewed the Repository Pattern but I didn't understand how to deal with some kind of these duplication.

Here is a sample of what I got here:

$orders = ( new OrderList() )
            ->where( 'order_state_id', '!=', null )
            ->where( 'address_id', '!=', null )
            ->where( 'user_id', '!=', null )
            ->where( 'id', '>', $this->idLimitation )
            ->whereHas( 'orderDetail', function ( $query ) use ( $supplier_mega ) {
                //In case he is not the super user supplier check for conditions
                if ( Auth::guard( 'supplier' )->user()->super_user != 1 ) {
                    if ( Auth::guard( 'supplier' )->user()->role == 1 ) {
                        $query->where( 'supplier_id', Auth::guard( 'supplier' )->user()->id );
                    } elseif ( in_array( Auth::guard( 'supplier' )->user()->role, [ 2, 3, 4 ] ) ) {
                        $supplier_id = ( new Supplier() )->where( 'id', Auth::guard( 'supplier' )->user()->accountant_id )->first();
                        $query->where( 'supplier_id', $supplier_id->id );
                    }
                }

Now starting from ->whereHas( 'orderDetail', function ( $query ) use ( $supplier_mega ) { is repeated in this controller about 6 times but the rest of the function is not.

How can I deal with such duplication?

Another example here (upload multiple images):

if ( $request->hasFile( 'side_image' ) ) {
            foreach ( $request->side_image as $side_image ) {
                $input['campaign_ads_id'] = $campaignAdsId;
                $filename                 = Str::random( 10 ) . '.' . $side_image->getClientOriginalExtension();
                Image::make( $side_image )->save( 'images/campaignAds/' . $filename );
                $input['side_image'] = $filename;
                ( new CampaignAdsImage() )->create( $input );
            }
        }

This code is repeated in my code twice but of-course the some parts is different like the path where I store the images for instance.

I think you can DRY this up pretty well by refactoring.

This feels like a “ifs are evil” code smell to me. There are three “if” statements within that whereHas. They are packed in fairly tightly, and it makes that part of the code more difficult to read. Trying to refactor that as much as possible can resolve the duplication along the way, and ultimately make things much more readable and manageable.

Try to give every “if” it’s own method, if for no other reason than the opportunity to give it a very concise name explaining what it does. This greatly improves code readability.
The “if"s being repeated is a good indicator that you can flip the control around. Try to find a way to visit each “if” once and do all the things you need to do based on the results in one place. In this case, that means breaking the construction of the query into a couple of pieces.
“if"s are decision points. They are apt to be the focus in future debugging efforts. Push these decision points to as early in the process as you can, and obsess over making that code as simple and as readable as possible.
Auth::guard( 'supplier' )->user() gets resolved a number of times. Instead, resolve it once as early as possible and catch the results in a class attribute or parameter.
The “if/elseif” is overreaching; much of the content is the same. Narrow the scope to just resolving the required supplier id, encapsulate it in a method, and call that within the required context.  Furthermore, this looks to be a second important variable which gets used multiple times, so resolve this as early as possible as well and get it into an attribute or parameter.
Inline role ids is a bad idea. It’s possible that the database will change, or a problem will occur on data recover or seeding on deploy to a new environment, leaving different role to id mapping. Even a short exposure to this problem will mean non-admins could be logged in as admins. Further, using the full name inline instead of ids helps improve code readability as it tells more of a story. “This happens for admins” instead of “this happens for some user role, I could look in the database to know which...” Using the name adds a bit of overhead, but it can be eager loaded with the user and makes the code a lot more readable.
No need to load the accountant user record as we only need the id, which we have account_id anyway. Referencing that directly will save a query here, or more as this block is repeated.
Database field names can be used to help improve readability, too. $user->role_id would help identify that it’s a foreign key rather than a string. Likewise, putting ‘is_’ or ‘has_’ on the front of booleans helps make them quickly obvious, such as $user->is_superuser. Along with that, setting $casts = [‘us_superuser’ => ‘boolean’,] ; in the models lets you compare it against true/false instead of 1 or 0, which is a lot more readable.
whereNotNull(‘order_state_id’) is slightly more readable than where(‘order_state_id’, ‘!=’, null). This can be applied to a number of the where() clauses
Refactor this query builder into a “plain old php” class.  Call it a repository if you think it fits, but don’t get too hung up on the name. It could be as easy as a new app/Queries/OrderList.php folder and class.  That new class can be injected into the controller. Make it fluent, and abstract decisions out of it. The class should do the lifting, but be dumb. The calling entity should encapsulate the decision making and directing, and only that.
Replace eloquent with whereHas with query builder, joins, and where clauses. The eloquent and whereHas approach can certainly still work and be much cleaner than it currently is, but I find query builder to be much more readable and consistent across different models and needs when the queries start to get complicated. Perhaps this one is down to a personal preference.

With all of that in mind, here’s where I would land given my understanding of the posted code.

app/Queries/OrderList:

<?php

namespace App\Queries;

use App\Models\OrderList;
use Illuminate\Support\Collection;
use Illuminate\Support\Facades\DB;

class OrderLists
{
    protected $query;

    public function buildQuery(int $idLimitation): self
    {
        $this->query = DB::table('order_lists')
            ->select([
                'order_lists.id as id',
                //etc
            ])
            ->whereNotNull('order_state_id')
            ->whereNotNull('address_id')
            ->whereNotNull('user_id')
            ->where('id', '>', $idLimitation);

        return $this;
    }

    protected function applyNonSuperUserClauses(int $requiredSupplierId):self
    {
        $this->query->leftJoin('order_details', 'order_lists.id', '=', 'order_details.order_list_id');
        $this->query->where('order_details.supplier_id', '=', $requiredSupplierId);

        //Other  join and where clauses as appropriate

        return $this;
    }

    protected function fetchResults():Collection{

        return $this->query->get();
    }

controller:

 public function index(OrderList $orderListQuery)
    {
        $user = Auth::guard('supplier')->user();

        $idLimitation = $this->fetchIdLimitation();

        if ($this->isSuperUser($user)) {
            return view('your.view', [
                'orderList' => $orderListQuery
                    ->buildQuery($idLimitation)
                    ->fetchResults(),
            ]);
        }

       //More secured version is the default behavior.
        return view('your.view', [
            'orderList' => $orderListQuery
                ->buildQuery($idLimitation)
                ->applyNonSuperUserClauses($this->getRequiredSupplierIdByRole($user))
                ->fetchResults(),
        ]);

    }

 protected function isSuperUser($user): bool
    {
        return $user->is_super_user;
    }

 protected function getRequiredSupplierIdByRole($user): ?int
    {
        if ($this->hasAdminRole()) {
            return $user->id;
        }

        if ($this->hasLimitedUserRole()) {
            return $user->accountant_id;
        }
        return null;
    }

 //'Admin' and 'LimitedUser' based on assumptions about role ids 1,2,3,4.
    // Change these to be as concise as possible.
    protected function hasAdminRole($user): bool
    {
        return (in_array($user->role->name, $this->getAdminRoles()));
    }

    protected function getAdminRoles(): array
    {
        return [
            'Admin',
        ];
    }

    protected function hasLimitedUserRole($user): bool
    {
        return (in_array($user->role->name, $this->getLimitedRoles()));
    }

    protected function getLimitedRoles(): array
    {
        return [
            'Middle Manager',
            'Secretary',
            'Janitor',
        ];
    }

This turned into a long reply. Hopefully something here helps!


Calculating sum of similarities of strings Puzzle:

  For two strings A and B, we define the similarity of the strings to be
  the length of the longest prefix common to both strings. For example,
  the similarity of strings "abc" and "abd" is 2, while the similarity
  of strings "aaa" and "aaab" is 3. Calculate the sum of similarities of
  a string S with each of its suffixes.

  Explanation:

  For ababaa the suffixes of the string are "ababaa", "babaa", "abaa",
  "baa", "aa" and "a". The similarities of each of these strings with
  the string "ababaa" are 6,0,3,0,1,1 respectively. Thus the answer is 6
  + 0 + 3 + 0 + 1 + 1 = 11.

How can I improve execution time of this (when larger testcases are provided)?

import java.util.Scanner;
public class Solution {
    public static void main(String[] args) {
        Scanner scanner;
        Solution sr = new Solution();
        scanner = new Scanner(System.in);
        int no_cases = scanner.nextInt();
        for (int i = no_cases; --i >=0; ) {
            String to_proc = scanner.next();
            sr.solve(to_proc);
        }
    }
    private void solve(String to_proc) {
            String str=to_proc;
            int len=to_proc.length();
            int count=0;
            int total=0;
            for(int i=1;i<len;i++)
            {
                count=0;
                for(int j=i;j<len;j++)
                {
               //     System.out.println(str.charAt(j)+" , "+str.charAt(j-i));
                    if(str.charAt(j-i)==str.charAt(j))
                    {
                        count++;
                    }
                    else
                    {
                        break;
                    }
                }
                total=total+count;
            }
            System.out.println(total+len);
    }
}

I think you need a suffix tree.

Its a data structure that puts all the prefixes of a string in a tree. This makes a number of operations faster, and I think you can make your similarity metric qualify.


Assign values to a matrix based on the location of non-zero elements in another matrix I have a matrix, A containing ones and zeros. I want to create a new matrix B where the non-zero elements are in the same position as those in A, but where the values are given by the location of the first non-zero element in that row.

A few examples to show what I mean:

1  1  0  0   ->  1  1  0  0   (the first non-zero element is in column 1)
0  1  0  1   ->  0  2  0  2   (the first non-zero element is in column 2)
0  0  1  0   ->  0  0  3  0   (the first non-zero element is in column 3)
0  0  0  1   ->  0  0  0  4   (the first non-zero element is in column 4)

The code I have now is:

for i = 1:size(A,1)
for j = 1:size(A,2)
if A(i,j) == 1
B(i,:) = A(i,:) * find(A(i,:) == 1,1);
end
end
end

This gives the following results for a given A:

A
A =
     0     0     1     0
     0     1     1     1
     0     0     0     1
     1     1     1     1
     1     1     0     1
     0     1     1     1
     1     1     1     1
     1     1     1     1
     0     0     1     1
     0     0     1     1
B
B =
     0     0     3     0
     0     2     2     2
     0     0     0     4
     1     1     1     1
     1     1     0     1
     0     2     2     2
     1     1     1     1
     1     1     1     1
     0     0     3     3
     0     0     3     3

This code is slow if A is big. Is there a way to improve this code? I'm looking for improvements both on performance, and on the coding style / best practices etc.

First, let's go through the code:

You have two calls to size() in the beginning of your code, where the dimension is specified. It's better to make a single call to size, and save the two variables.
This won't improve performance much, but will make it possible to reuse the two dimensions:

[rows, cols] = size(A);

You are using i and j as names for the iterators. In general, using those names are discouraged. It is not a problem when working with only real numbers, but once you
start working with complex numbers this could be a problem. As an example, consider the following code, where we're adding complex numbers:

x = 1 + i;      % Complex number, Real(x) = 1, Imag(x) = 1
for i = 1:10
   x = x + i
end

In this case, x will be 11 + i, instead of 10 + 11*i. Similarly, you might get code that doesn't behave as expected, if you forget to intialize i to a variable. For instance,
if you think you have written i = true, i = false, i = 0 or something similar, neither of the two statements will give an error: i == true, i == false. This might be hard to debug
since you don't get any error messages.

You have a growing loop inside your code. A row will be added to B for each iteration of the outer loop. This is very slow, and MATLAB actually warns you about this:

  The Variable A appears to change size on every loop iteration

You should always preallocate memory. In this case, the size of the B matrix is known:

B = zeros(size(A));
% or
B = zeros(rows, cols);

You loop through all columns and use if A(i,j) == 1 to check if there are any non-zero elements. In the following code you use find to find the index of that element. A better
way to do this is:

for ii = 1:rows
    idx = find(A(ii,:) == 1, 1);
    if ~isempty(idx)
       B(ii,:) = A(ii,:) * idx;
    end
end

This way, you can save one loop, and only loop through each element once.

You should use proper indentation. This can easily be achieved in Matlab. Press Ctrl+a followed by Ctrl+i, and Matlab will do the
indentation for you.

The better way to do this:

A better way to do this, that's both simpler and faster is:

% Find the column index of the first element in each "slice".
[~, idx] = max(A,[],2);

% Multiply the column index with each row of the initial matrix
bsxfun(@times, A, idx);

[M, idx] = max(A, [], dim) returns the value and index of the largest element along one dimension. It will only give one index per row or column (dependent on which dimension you choose).
We can now multiply the vector of indices by the elements in A. For this, we use bsxfun.