Abstract factory using generics I am learning design patterns and I have implemented the abstract factory using generics C#.NET. Is this implementation correct or not? Or is there any SOLID principle violation?

using System;
namespace GenericAbstractFactory
{
    public enum vehicles { car, bicycle, truck }

    interface IFourWheeler
    {
        void DriveFourWheeler();
    }

    interface ITwoWheeler
    {
        void DriveTwoWheeler();
    }

    interface IVehcileAbstractFactory<VehicleTypeFactory> //Abstract Factory
    {
        VehicleTypeFactory GetVehicleType();
    }

    class FourWheeler<VehicleType> : IVehcileAbstractFactory<IFourWheeler>
        where VehicleType : IFourWheeler, new() // ConcreteFactory
    {
        public IFourWheeler GetVehicleType()
        {
            return new VehicleType();
        }
    }

    class TwoWheeler<VehicleType> : IVehcileAbstractFactory<ITwoWheeler>
        where VehicleType : ITwoWheeler, new() // ConcreteFactory
    {
        public ITwoWheeler GetVehicleType()
        {
            return new VehicleType();
        }
    }

    class Car : IFourWheeler
    {
        public void DriveFourWheeler()
        {
            Console.WriteLine("Drive Car");
        }
    }

    class Truck : IFourWheeler
    {
        public void DriveFourWheeler()
        {
            Console.WriteLine("Drive Truck");
        }
    }

    class Bicycle : ITwoWheeler
    {
        public void DriveTwoWheeler()
        {
            Console.WriteLine("Drive Bicycle");
        }
    }

    class Program
    {
        static void Main(string[] args)
        {
            vehicles veh = vehicles.bicycle;
            IVehcileAbstractFactory<IFourWheeler> getFourWheeler = null;
            IVehcileAbstractFactory<ITwoWheeler> getTwoWheeler = null;
            IFourWheeler actionFourWheeler = null;
            ITwoWheeler actionTwoWheeler = null;
            if (veh == vehicles.car)
            {
                getFourWheeler = new FourWheeler<Car>();
                actionFourWheeler = getFourWheeler.GetVehicleType();
            }
            else if (veh == vehicles.bicycle)
            {
                getTwoWheeler = new TwoWheeler<Bicycle>();
                actionTwoWheeler = getTwoWheeler.GetVehicleType();
            }
            else if (veh == vehicles.truck)
            {
                getFourWheeler = new FourWheeler<Truck>();
                actionFourWheeler = getFourWheeler.GetVehicleType();
            }
            else
            {
                throw new Exception("vehicle type not supported");
            }

            if (actionFourWheeler != null)
            {
                actionFourWheeler.DriveFourWheeler();
            }
            else if (actionTwoWheeler != null)
            {
                actionTwoWheeler.DriveTwoWheeler();
            }
            Console.Read();
        }
    }
}

I'm not sure, but I don't think a generic approach is suitable for the abstract factory pattern, because the client has to specify the concrete class type in order to call the factory, and then the whole point of that pattern is gone?

The way you've tried to implement it works, but you have effectively two parallel factories (one for four wheels, and one for two) that have nothing or just very little to do with each other (DriveFourWheeler() vs DriveTwoWheeler()).

Below I've tried to fiddle a little bit with you code in order to show my understanding of the pattern and still with a generic touch.

using System;
namespace GenericAbstractFactory
{
  public enum Vehicles { car, bicycle, truck }

  interface IWheeler
  {
    void Drive();
  }

  interface IFourWheeler : IWheeler
  {
  }

  interface ITwoWheeler : IWheeler
  {
  }

  interface IVehicleAbstractFactory
  {
    IWheeler GetVehicle();
  }

  class FourWheelerFactory<FourWheeler> : IVehicleAbstractFactory where FourWheeler: IFourWheeler, new()
  {
    public IWheeler GetVehicle()
    {
      return new FourWheeler();
    }
  }

  class TwoWheelerFactory<TwoWheeler> : IVehicleAbstractFactory where TwoWheeler: ITwoWheeler, new()
  {
    public IWheeler GetVehicle()
    {
      return new TwoWheeler();
    }
  }

  class Car : IFourWheeler
  {
    public void Drive()
    {
      Console.WriteLine("Drive Car");
    }
  }

  class Truck : IFourWheeler
  {
    public void Drive()
    {
      Console.WriteLine("Drive Truck");
    }
  }

  class Bicycle : ITwoWheeler
  {
    public void Drive()
    {
      Console.WriteLine("Drive Bicycle");
    }
  }

  class Program
  {
    static IVehicleAbstractFactory GetFactory(Vehicles vehicle)
    {
      switch (vehicle)
      {
        case Vehicles.car:
          return new FourWheelerFactory<Car>();
        case Vehicles.truck:
          return new FourWheelerFactory<Truck>();
        case Vehicles.bicycle:
          return new TwoWheelerFactory<Bicycle>();
      }
    }

    static void Main(string[] args)
    {
      IVehicleAbstractFactory factory = GetFactory(Vehicles.bicycle);
      IWheeler vehicle = factory.GetVehicle();
      vehicle.Drive();

      Console.Read();
    }
  }
}

I'm not saying this is the right way for the factory pattern, but may be a little clearer. The client still has to know which actual class he wants to create due to the nature of generics.


Reacting to Boolean state in reactive ViewModels When this ViewModel loads we kick off the IsAuthenticatedCommand:

If the result of IsAuthenticatedCommand is false we set the value of NeedsToLogin to true.
If the result of IsAuthenticatedCommand is true we set the value of IsAuthenticated to true.

There are (subscriptions?) to both NeedsToLogin and IsAuthenticated that are waiting for the true value.

Is there a better way to handle this if/else logic?
Should I be worried about race conditions?

public class LoadingViewModel : ReactiveObject, IRoutableViewModel {

    private bool _isAuthenticated = false;

    public bool IsAuthenticated {
    get { return _isAuthenticated; }
    set { this.RaiseAndSetIfChanged (ref _isAuthenticated, value); }}

    private bool _needsToLogin;

    public bool NeedsToLogin {
    get { return _needsToLogin; }
    set { this.RaiseAndSetIfChanged (ref _needsToLogin, value); }}

    public ReactiveCommand<bool> IsAuthenticatedCommand { get; protected set; }

    public LoadingViewModel (IScreen screen)
    {
        HostScreen = screen;

        this.WhenAnyValue (x => x.IsAuthenticated)
            .Where (x => x == true)
            .Subscribe (x => { HostScreen.Router.Navigate.ExecuteAsyncTask (new OnboardingViewModel (HostScreen));});

        this.WhenAnyValue (x => x.NeedsToLogin)
            .Where (x => x == true)
            .Subscribe (_ => { HostScreen.Router.Navigate.ExecuteAsyncTask (new LoginViewModel (HostScreen)); });

        //always return false for testing.
        IsAuthenticatedCommand = ReactiveCommand.CreateAsyncTask (async (_) => await Task.Run (() => {return false;}));

        IsAuthenticatedCommand.Subscribe (_ => {
        if (_) { this.IsAuthenticated = true;} else {this.NeedsToLogin = true;}
        });
    }
}

You need to format your code neatly; this will improve readability, which will decrease bugs.  First, you have this:

public bool IsAuthenticated {
get { return _isAuthenticated; }
set { this.RaiseAndSetIfChanged (ref _isAuthenticated, value); }}

This should be:

public bool IsAuthenticated
{
    get { return _isAuthenticated; }
    set { this.RaiseAndSetIfChanged (ref _isAuthenticated, value); }
}

The if/else statement should be written like this:

IsAuthenticatedCommand.Subscribe (_ => {
    if (_) { this.IsAuthenticated = true; }
    else { this.NeedsToLogin = true; }
});

Also, you need to be consistent with your spacing; sometimes you have a space between braces, sometimes you don't:

if (_) { this.IsAuthenticated = true;} else {this.NeedsToLogin = true;}

I am no expert at lambda statements, but your lambda with the if/else looks fine to me.


Simple password dictionary attack script This is my simple Perl script implement password dictionary attacking, encryption algorithm SHA1 with different length.

It takes 2 arguments as input: first argument is password dictionary file path, the second one is a text file contains line-by-line encrypted text. I used 2 common dictionaries for testing: rockyou and phpbb.

Any suggestions to make it more efficient?

#!/usr/bin/perl
use strict;
use warnings;
use Digest::SHA;
use autodie qw(open close);

# command arguments
my $dic = $ARGV[0];
my $pwd = $ARGV[1];

# read input cipher texts and dictionary
open (my $input, '<', $pwd);
my @ciphers = <$input>;
# remove end of line characters
chomp(@ciphers);

print "\n";
print "dictionary attack demo\n";
print "encrypt algorithm: SHA1\n";
print "======================================\n\n";

foreach my $cipher(@ciphers){
    my $password = "*";
    my $count = 0;
    my $factor = 0;

    # calculate factor depends on length of cipher text
    $factor = length($cipher);
    if ($factor == 40) {
        $factor = 1;
    }
    elsif(($factor == 56) ||
          ($factor == 64) ||
          ($factor == 96) ||
          ($factor == 128))
    {
        $factor = $factor * 4;
    }
    else {
        next;
    }

    # create new SHA algorithm object with key length is factor
    my $sha = Digest::SHA->new($factor);
    open (my $dict, '<', $dic);

    while(<$dict>){
        chomp;
        $sha->add($_);
        # encrypt each word with SHA1 algorithm
        my $encrypt_text = $sha->hexdigest;

        # compare with cipher text, if matches
        if($encrypt_text eq $cipher){
            $password = $_;
            last;
        }
        # count number of tested words
        $count++;
    }

    # print out result, "*" if not found
    print "$cipher\:\:$password\t[ATTEMPT\:\:$count words]\n";
    # close dictionary file handle
    close ($dict);
}

# close input file handle
close($input);

Performance

For each cipher, you re-read and re-encrypt the dictionary again and again. If some of your ciphers have the same length, then this is wasteful. At the minimum, you should load the dictionary only once. If you have a lot of passwords, then you could pre-encrypt all words in the dictionary for each unique factor. That should speed things up.

Coding style

You could simplify this if condition:

if (($factor == 56) ||
      ($factor == 64) ||
      ($factor == 96) ||
      ($factor == 128))
{ ... }

by using a helper function, perhaps something like this:

sub one_of {
    my $value = shift;
    for (@_) { return 1 if $_ eq $value; }
}
# ...
if (&one_of($factor, 56, 64, 96, 128)) { ... }

This can be slightly simpler:

my @ciphers = <$input>;
# remove end of line characters
chomp(@ciphers);

like this:

chomp(my @ciphers = <$input>);

And then close($input) right after. It's good to close an input file immediately after you're done reading from it.

You could simplify $factor = $factor * 4; as $factor *= 4;

I think all your comments are pointless. You can remove them and let the code speak for itself.

If the meaning of a variable is not clear enough without a comment, then try to rename it to be self-explanatory. Note that the variable will be self-explanatory every time you use it, whereas a comment only explains it in once place.

Likewise, you can extract non-trivial logic to a function with a name that explains what it does. For example:

sub get_factor {
    my $cipher = shift;
    my $factor = length($cipher);
    if ($factor == 40) {
        return 1;
    } elsif (&one_of($factor, 56, 64, 96, 128)) {
        return $factor * 4;
    }
}

foreach my $cipher(@ciphers) {
    # ...
    my $factor = &get_factor($cipher) || next;
    # ...
}


Mean of combined sublists with different lengths I have a seemingly simple issue. Consider this list:

a = [[12.0, 5.0, 63.0], [0.1, 2.0, 7.1, 3.0, 2.3, 5.0, 8.4]]

I want to find the mean (using numpy) of all the elements in its sublists combined. In this case the result should be:

10.79

obtained as:

np.mean([12.0, 5.0, 63.0, 0.1, 2.0, 7.1, 3.0, 2.3, 5.0, 8.4])

The solution I've found is to flatten the list first and then obtain the mean, as:

np.mean([item for sublist in a for item in sublist])

but this seems unnecessarily complicated. I would've assumed that numpy.mean() could handle this case without the need to modify the list first. I tried using the argument axis to no avail.

Am I missing some functionality here?

Assuming your nesting is only one level deep, the concatenation can be done very easily using np.hstack.  This will treat the inner lists as vectors, then concatenate them end-to-end as a 1D numpy array.  You can then take the mean of the resulting array.  So this will do what you want:

>>> np.hstack(a).mean()
10.79

Or equivalently (but more verbose in my opinion):

>>> np.mean(np.hstack(a))
10.79


Reading a string-to-string map from an archive I have a CArchive Which contains a serialized CMapStringToString.

From my research it is my understanding that it is quite difficult to directly read a CArchive from within a C# DLL. So I decided to read it in the MFC application and feed it into the DLL interface.

At the moment I am doing the following:

m_pInterface->UpgradeDatabase();

CFile           fileNotes;
CFileException  ex;
TCHAR           szError[_MAX_PATH];
CMapStringToString mapSSBrotherNotes;

if (fileNotes.Open(_T("d:\\BrotherNotes.BIN"), CFile::modeRead, &ex))
{
    TRY
    {
        CArchive ar(&fileNotes, CArchive::load);
        mapSSBrotherNotes.Serialize(ar);
        ar.Close();

        POSITION sPos = mapSSBrotherNotes.GetStartPosition();
        while (sPos != NULL)
        {
            CString strName, strNotes;
            CComBSTR bstrName, bstrNotes;

            mapSSBrotherNotes.GetNextAssoc(sPos, strName, strNotes);

            bstrName = strName.AllocSysString();
            bstrNotes = strNotes.AllocSysString();

            m_pInterface->SetPublisherNotes(bstrName, bstrNotes);
        }
    }
    CATCH(CException, pEx)
    {
        pEx->GetErrorMessage(szError, _MAX_PATH);
        AfxMessageBox(szError, MB_OK | MB_ICONERROR);
    }
    END_CATCH

    fileNotes.Close();
}
else
{
    // If the file was not found, this is probably not an error
    if (ex.m_cause != CFileException::fileNotFound)
    {
        ex.GetErrorMessage(szError, _MAX_PATH);
        AfxMessageBox(szError, MB_OK | MB_ICONERROR);
    }
}
m_pInterface->SavePublisherData(&iResult);

It works fine but is there a better way to read this data file from within the C# DLL?

Side question - can my use of CString / CComBSTR be simplified in my current code?

But is there a better way to read this data file from within the C# DLL?

You can write a C++/CLI wrapper. It'll compile as a .NET assembly that you can reference in your project as any other .NET dll but inside it'll be able to work with `C++ and use your current code as it is.

Visual C++ supports interoperability features that allow managed and unmanaged constructs to co-exist and interoperate within the same assembly, and even in the same file.
Mixed assemblies are capable of containing both unmanaged machine instructions and MSIL instructions. This allows them to call and be called by .NET components, while retaining compatibility with components that are entirely unmanaged. Using mixed assemblies, developers can author applications using a mixture of managed and unmanaged functionality. This makes mixed assemblies ideal for migrating existing Visual C++ applications to the .NET Platform.

Mixed (Native and Managed) Assemblies
I recommend reading

C++/CLI in Action and/or
Expert Visual C++/CLI: .NET for Visual C++ Programmers


Leetcode Mountain Array I've been working on longest mountain in array on Leetcode:

  Let's call any (contiguous) subarray B (of A) a mountain if the following properties hold:

  B.length >= 3
  There exists some 0 < i < B.length - 1 such that B[0] < B[1] < ... B[i-1] < B[i] > B[i+1] > ... > B[B.length - 1]

  (Note that B could be any subarray of A, including the entire array A.)

  Given an array A of integers, return the length of the longest mountain.

  Return 0 if there is no mountain.

  Example:

  Input: [2,1,4,7,3,2,5]
  Output: 5
  Explanation: The largest mountain is [1,4,7,3,2] which has length 5.

I've come up with a solution that passes the test but only beats 17% of javascript submissions. I just want to know if there's anything I can do with my code to optimize it's performance. Also please let me know if the code could be written better.

The idea is as followed. In a mountain array we have a start, peak, and an end. I keep track of when a mountain starts, when a mountain peaks and when a mountain ends. When the mountain ends i pop off my start value from the start stack and use the spread operator to add it to a result array which contains the peak and the end of a mountain.

For example the array [1,3,8]... the mountain starts at index 1, peaks at index 3 and ends at index 8. In order to find the length of the array I then subtract the end from the start.

A mountain can have multiple peaks and thus an array such as [[1,3,8],[8,13,18]] is possible. I'm handling this situation by having a maxDifference variable to store the maximum differences if a mountain is complete [start, peak, end].

var longestMountain = function(A) {

    let maxDifference = 0;
    let startPeakEnd = [];
    const start = [];
    const innerLoop = [];

    if(A[1]>A[0])start.push(0)
    else start.push(1);

    for(let i = 0; i < A.length; i++){
        if(A[i-1]<A[i] && A[i+1]<A[i]){
            startPeakEnd.push(i);
        }else if(A[i-1]>=A[i] && A[i+1]>=A[i]){
            startPeakEnd.push(i);
            innerLoop.push([start.pop(),...startPeakEnd]);
            start.push(i)
            startPeakEnd = [];
        }else if(A[i+1]===undefined){
            startPeakEnd.push(i);
            innerLoop.push([start.pop(),...startPeakEnd]);
        }
    }

    for(let item of innerLoop){
        if(item.length===3){
            if(item[2]-item[0]+1>maxDifference){
                maxDifference=item[2]-item[0]+1;
            }
        }
    }

    return maxDifference;

};

It would be better to try and use built in functions for iterating over arrays. It appears that you are trying to generate and store all possible mountains. This would not be necessary. This would be my approach.

var longestMountain = function(arr) {
  //get nodes that may be the peaks of mountains
  var candidates = arr.map(function(currentValue, index) {
    if (index == 0 || index === arr.length - 1) {
      return false;
    } else {
      return arr[index - 1] < currentValue && arr[index + 1] > currentValue;
    }
  });

  //for each index, calculate the height of the slope where arr[i - 1] < arr[i]
  var increasingMountainCounts = arr.map(function(currentValue, index) {
    if (index == 0 || arr[index - 1] >= currentValue) {
      return 0;
    } else {
      return arr[index - 1] + 1;
    }
  });

  //for each index, calculate the height of the slope where arr[i - 1] > arr[i]
  var decreasingMountainCounts = arr.reverse().map(function(currentValue, index) {
    if (index == 0 || arr[index - 1] >= currentValue) {
      return 0;
    } else {
      return arr[index - 1] + 1;
    }
  });

  //for each candidate peak, get the height of the mountain with that peak
  var maxMountainHeight = 0;

  candidates.forEach(function(currentValue, index) {
    if (currentValue !== true) {} else {
      maxMountainHeight = Math.max(maxMountainHeight, increasingMountainCounts[index - 1] + decreasingMountainCounts[index + 1] + 1);
    }
  });

  return maxMountainHeight;

};

An alternative approach which uses more javascript array iterators but in my opinion makes the code a bit less readable.

var longestMountain = function(arr) {
  var candidates = arr.map(function(currentValue, index) {
    if (index == 0 || index === arr.length - 1) {
      return false;
    } else {
      return arr[index - 1] < currentValue && arr[index + 1] > currentValue;
    }
  });

  var increasingMountainCounts = arr.map(function(currentValue, index) {
    if (index == 0 || arr[index - 1] >= currentValue) {
      return 0;
    } else {
      return arr[index - 1] + 1;
    }
  });

  var decreasingMountainCounts = arr.reverse().map(function(currentValue, index) {
    if (index == 0 || arr[index - 1] >= currentValue) {
      return 0;
    } else {
      return arr[index - 1] + 1;
    }
  });

  var maxMountainHeightAccumulator = (maxMountainHeight, currentValue, index) => {
    if (currentValue !== true) {
      return maxMountainHeight;
    } else {
      return Math.max(maxMountainHeight, increasingMountainCounts[index - 1] + decreasingMountainCounts[index + 1] + 1);
    }
  };

  return candidates.reduce(maxMountainHeightAccumulator, 0);

};

In terms of improving performance, this reminds me of the problem of finding the (contigious) subsequence with the maximum sum/product. That solution would probably require completely rewriting your code.


Printing the right diagonal of a matrix I am solving a coding challenge whose objective is to print the right diagonal of a matrix. And below is my code to do that.

package basics;
import java.util.*;
/**
Problem statement : Print right diagonal of a matrix. Example: For input : a b, c d ; the right diagonal is b c
**/
public class PrintRightDiagonal{

    private int rowSize;
    private int colSize;
    private char[][] matrix;
    private Scanner scanner = new Scanner(System.in);
    private String[] rows;
    private String userInput;

    public static void main(String[] args){
        PrintRightDiagonal printRightDiagonal = new PrintRightDiagonal();
        printRightDiagonal.readMatrixDimensions();
        printRightDiagonal.readInputEntries();
        printRightDiagonal.validateInputEntries();
        printRightDiagonal.insertIntoArr();
        printRightDiagonal.findRDiagonal();
    }

    private void readMatrixDimensions(){
        System.out.println("Please enter the rowsize of the 2D array");
        rowSize = scanner.nextInt();
        System.out.println("Please enter the colsize of the 2D array");
        colSize = scanner.nextInt();
        if(rowSize!=colSize){
            System.err.println("Cannot find the diagonal of a matrix because the rowsize and column size are different");
            System.exit(0);
        }
    }

    private void readInputEntries(){
        System.out.println("Please enter the rows separated by comma. Example if the matrix is a 2x2 enter i/p as ab,cd");
        scanner.nextLine();
        userInput = scanner.nextLine();
    }

    private void validateInputEntries(){
        rows = userInput.split(",");
        if(rows.length!=rowSize){
            System.err.println("Entered rows doesn't match with the rowSize");
            System.exit(0);
        }
        for(String row : rows){
            if(row.length()!=colSize){
                System.err.println("Entered cols doesn't match with the colsize");
                System.exit(0);
            }
        }
    }

    private void insertIntoArr(){
        //Initialize matrix
        matrix = new char[rowSize][colSize];
        int index=0;
        for(String row : rows){
            matrix[index++] = row.toCharArray();
        }
        //print matrix once
        System.out.println("Here is your matrix.");
        for(int i=0; i<rowSize; i++){
                for(int j=0; j<colSize; j++){
                    System.out.print(matrix[i][j] +" ");
                }
                System.out.println("");
        }
    }

    private void findRDiagonal(){
        System.out.println("Here is the Right diagonal of the matrix.");
        for(int i=0,j=colSize-1; i<rowSize && j>=0; i++,j--){
                System.out.print(matrix[i][j] + " ");
        }
    }

}

The above works fine. But, I doubt if the code is readable, is it following an OOP way. I think the class name sounds like a method name. Given a code challenge like this how to ensure that we don't miss the basics like class naming, methods organization. In the above code I am not sure if the method organization is correct.
Consider the following pseudo code.

//pseudo code

    class Name{
     main(){
      a();
      b();
      c();
     }
    }

As in the above pseudo code, the method a(), b(), c() should be written above the main() method or below the main()?

And here is my thought to improve this code. Write a class which just has a main method and from it initialize another class and call its methods, so that main class duty is just to initialize the worker class and call its operations. But, do we really need 2 classes in this case?

Throw some light on this.

Sequential cohesion

From Code Complete, chapter 7, High-quality routines:

  Sequential cohesion exists when a routine contains operations that must be performed in a specific order, that share data from step to step, and that don’t make up a complete function when done together.

Consider these lines in the code:

printRightDiagonal.readMatrixDimensions();
printRightDiagonal.readInputEntries();
printRightDiagonal.validateInputEntries();
printRightDiagonal.insertIntoArr();
printRightDiagonal.findRDiagonal();

If you reorder these calls, the program will compile, but it won't work correctly. This is undesirable.

Consider this alternative:

Matrix matrix = readMatrix(new Scanner(System.in));
matrix.printRightDiagonal();

Where readMatrix does:

int size = readMatrixSize(scanner);
char[][] data = readMatrixEntries(scanner, size);
return new Matrix(data);

You can guess what readMatrixSize and readMatrixEntries do.
The point is, each function does one thing (functional cohesion),
the relationship between statements is clear,
and reordering the statements will result in compile errors,
protecting you from runtime errors.

Encapsulation

The idea of encapsulation is limiting the scope of low-level details (information hiding), to reduce the mental burden of understanding a program.

All these fields in the class are a mental burden:

private int rowSize;
private int colSize;
private char[][] matrix;
private Scanner scanner = new Scanner(System.in);
private String[] rows;
private String userInput;

They are a mental burden because these variables are accessible in the entire class, so if you want to know where they are read or modified, you have to read the entire program.

The fields rows and userInput are the worst offenders,
because they are only used during input.
They should not be exposed like this to the entire class,
but encapsulated by the input mechanism, hidden from the rest of the program.

Side effects, semantic rules, failure of encapsulation

This method suggests a deep flaw in the program:

private void readInputEntries(){
    System.out.println("Please enter the rows separated by comma. Example if the matrix is a 2x2 enter i/p as ab,cd");
    scanner.nextLine();
    userInput = scanner.nextLine();
}

What is the scanner.nextLine(); doing there?
It's there because in the earlier readMatrixDimensions call,
after reading the matrix dimensions,
the end of line character is not read.
This is a big problem, for several reasons:

Huge burden on the reader. In order to understand the purpose of this line, you have to read the caller of this method, and the calls before this method that affected scanner.
readMatrixDimensions is clearly not doing its job. It leaves the scanner in a state that's not ready for the next use. It forces the rest of the program to understand and remember the low-level details of its implementation.
If you remove this line, the method will look correct, perfectly fine, even though the program will be broken.

The fix is relatively simple. Make sure that methods don't have such invisible rules, hidden dependencies on the implementation details outside of them. In this example, if you move scanner.nextLine() to readMatrixDimensions, this problem disappears.

Usability, logic

There's a mix of usability and logical issues here:

System.out.println("Please enter the rowsize of the 2D array");
rowSize = scanner.nextInt();
System.out.println("Please enter the colsize of the 2D array");
colSize = scanner.nextInt();
if(rowSize!=colSize){
    System.err.println("Cannot find the diagonal of a matrix because the rowsize and column size are different");
    System.exit(0);
}

The messages talk about a 2D array, but that's an implementation detail. The data of a matrix could be stored in a 1D array.
Users don't have to know such low-level implementation details.

The program is designed to work with a square matrix.
So why give the user the opportunity to enter different row and column sizes? Ask to input a single size value. That will effectively prevent the user from entering invalid values.

The validation is insufficient. 0 or negative values will not work out well.

Exiting the program in case of invalid input is not very user friendly.
Consider asking until there is a valid value, for example:

private int readMatrixSize(Scanner scanner) {
    System.out.println("Please enter the size of the matrix");
    int size;
    while (true) {
        size = scanner.nextInt();
        if (size > 0) {
            break;
        }
        System.out.println("The size must be positive. Try again!");
    }
    // read the end of line character
    scanner.nextLine();
    return size;
}

Single responsibility principle

insertIntoArr does two things:

Creates a matrix
Prints the matrix

These should be separated into createMatrix and printMatrix methods.

Naming

findRDiagonal prints the diagonal. There's not much there to "find". So printRightDiagonal would be a better name.

Program control

It's generally a bad practice to call System.exit in methods other than main. This kind of control is best in the main method, there should be no other exit points. If at any point an invalid input occurs, it would be better to throw a custom exception, and let the main method handle it.


Connect 4 on a 4×4 board in Python I'm having some trouble improving the Connect 4 game that is made in Python. I was wondering if I could get rid of the global statements and pass the variables between parameters and return its values. Also to put all of this in a class. I was wondering if this is possible.

And any more improvements that I could use would be appreciated a lot. Be gentle since I'm a newbie with Python.

grids = [[0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0]]

check = []

user = 1

# THIS WAS VERSION ONE
# def GridBuilder(x,y):
#     Colum_Count_Str = x
#     Colum_Count = int(Colum_Count_Str)
#     Row_Count_Str = y
#     Row_Count = int(Row_Count_Str)
#
#     Grids = [] * Colum_Count
#
#     for Current_Grid_Insert in range (Colum_Count):
#        Grids.insert(Current_Grid_Insert,[0]*Row_Count)
#
#     return Grids

class FullCapacity_error(Exception):
    pass

def user_def():
    #Change the global vartiables into local variables
    #Define them in the parameters of the functions.
    global user
    if user < 2:
        user = 2
    else:
        user = 1
    return user

def FullCapacity():
    global FullCapacity_error
    while True:
        try:
            if grids[3][userInput - 1] != 0:
                raise FullCapacity_error
            else:
                break
        except FullCapacity_error:
            print("This row is full, try another one!")
            confirmed()

def confirmed():
    PlayTheGame = True
    while PlayTheGame:
        try:
            global userInput
            userInput = int(input("\n Input a coin, player " + str(user) + "(1,4)\n"))
            if 5 > userInput > 0:
                PlayTheGame = False
            else:
                print("This number exceeds the board value.")
        except ValueError:
            print("This number exceeds the board value.")

def placement_def():
    for i in range(0, 4):
        counter = 0
        FullCapacity()
        if grids[i][userInput - 1] == 0:
            grids[i][userInput - 1] = int(user)
            print("\n Current Board\n", grids[3], "\n", grids[2], "\n", grids[1], "\n", grids[0])
            break

def check_def():
    global loop
    global check
    for i in range(0, 4):
        for a in range(0, 4):
            check.append(grids[i][a])
        if check == [1, 1, 1, 1] or check == [2, 2, 2, 2]:
            print("player " + str(user) + " has won")
            loop = False
            return loop

        else:
            check = []
    for i in range(0, 4):
        for a in range(0, 4):
            check.append(grids[a][i])
        if check == [1, 1, 1, 1] or check == [2, 2, 2, 2]:
            print("player " + str(user) + " has won")
            loop = False
            return loop

        else:
            check = []

def checkEmpty_def():
    global check
    for i in range(0, 4):
        for a in range(0, 4):
            check.append(grids[i][a])
    if 0 not in check:
        print("full")

def checks_def():
    return check_def() and checkEmpty_def() and diagcheck_def()

def diagcheck_def():
    global loop
    global check
    check = []
    diag = 0
    for i in range(0, 4):
        check.append(grids[diag][diag])
        diag = diag + 1
        if check == [1, 1, 1, 1] or check == [2, 2, 2, 2]:
            print("player " + str(user) + " has won")
            loop = False
            return loop

    check = []
    diag = 3
    diag2 = 0
    for i in range(0, 4):
        check.append(grids[diag][diag2])
        if check == [1, 1, 1, 1] or check == [2, 2, 2, 2]:
            print("player " + str(user) + " has won")
            loop = False
            return loop

loop = True

while loop:
    check_def()
    confirmed()
    placement_def()
    checks_def()
    if not loop:
        break
    user_def()

Design

You've got the right diagnosis: your functions are poorly designed, such that it is unclear what their inputs and outputs are.  We definitely need to fix your functions so that they act the way functions should.  Redesigning the code to be object-oriented would not necessarily improve the code further, though.

Let's consider your main loop:

loop = True

while loop:
    check_def()
    confirmed()
    placement_def()
    checks_def()
    if not loop:
        break
    user_def()

Looking at just that code, I can't tell what your functions do.  I can't even tell that it's code for a board game.  Compare that to the main() function in my solution below:

def main():
    board = [
        [0, 0, 0, 0],
        [0, 0, 0, 0],
        [0, 0, 0, 0],
        [0, 0, 0, 0],
    ]
    print_board(board)
    for player in itertools.cycle([1, 2]):
        make_move(board, player)
        print_board(board)
        if is_winner(board, player):
            print("Player {0} has won".format(player))
            break
        if is_board_full(board):
            print("Board is full")
            break

if __name__ == '__main__':
    main()

This code makes it clear that it's a board game in which two players take turns making a move.  Each function has:

A purpose, and the name tells you what it does.  check_def doesn't mean anything, but is_winner does.  The fact that the name starts with "is_…" implies that it's a function that returns either True or False.
A useful return value.  If is_winner(…) returns True, then the if statement in the loop can use that information to end the game.  You don't want the information to be passed back indirectly by setting a global loop variable.
Inputs passed as parameters.  What information does is_winner(…) need to do its job?  Obviously, it needs to know the state of the board.  It's also helpful to know which player just moved, so that it only needs to consider the most recent player's coins.

To help enforce these good habits, you should write a docstring for every function that explains the purpose, the inputs, and the return value.

Bug

Your checks_def() function, which is intended to detect the end of the game, is buggy:

def checks_def():
    return check_def() and checkEmpty_def() and diagcheck_def()

check_def() (which detects horizontal and vertical wins) never returns True: it can either return False, or fall off the end of the function and implicitly return None.  Therefore, due to the way short-circuit evaluation works with the and operator, the code checkEmpty_def() (to detect a tie) and diagcheck_def() (to detect a diagonal win) will never be executed.

The logic in that line is further wrong in that you should check for diagonal wins before detecting that the board is full, because the last coin that fills the board might be a winning move.

Expressiveness

Python has some very powerful techniques to write loops with a lot less code.  In the solution below, my is_winner(…) and is_board_full(…) functions make use of the built-in functions all() and any() with generator expressions.

zip(*board) is a handy idiom for transposing row and columns.

Suggested solution

from itertools import cycle

def print_board(board):
    """
    Print the board.
    """
    print("\nCurrent board:")
    for row in reversed(board):
        print(row)

def make_move(board, player):
    """
    Ask the player for a column, and mark the move on the board if it is a
    legal move.  If it is an illegal move, ask again.
    """
    while True:
        try:
            c = -1 + int(input("Input a coin, player {0} (1-4): ".format(player)))
            if not (0 <= c < 4):
                print("Column number is out of range.")
            top_row = next(row for row in board if row[c] == 0)
            top_row[c] = player
            break
        except ValueError:
            print("Illegal move.")
        except StopIteration:
            print("This column is full.  Try another one!")

def is_winner(board, player):
    """
    Return True if a row, column, or diagonal consisting of four entries for
    the given player.
    """
    return (
        any(all(c == player for c in row) for row in board) or
        any(all(r == player for r in col) for col in zip(*board)) or
        (board[0][0] == board[1][1] == board[2][2] == board[3][3] == player) or
        (board[0][3] == board[1][2] == board[2][1] == board[3][0] == player)
    )

def is_board_full(board):
    """
    Return True if all elements of the board are non-zero.
    """
    return all(all(row) for row in board)

def main():
    board = [
        [0, 0, 0, 0],
        [0, 0, 0, 0],
        [0, 0, 0, 0],
        [0, 0, 0, 0],
    ]
    print_board(board)
    for player in cycle([1, 2]):
        make_move(board, player)
        print_board(board)
        if is_winner(board, player):
            print("Player {0} has won".format(player))
            break
        if is_board_full(board):
            print("Board is full")
            break

if __name__ == '__main__':
    main()


What is the best way to code an array that has same keys as another array's values I have two arrays as follows:

public $headers = [
    'user_id',
    'timestamp',
    'value',
];

protected $args = array('user_id' => array('filter' => FILTER_VALIDATE_REGEXP,
                                                      'options' => array('regexp' => '/^[a-zA-Z0-9_]*$/'), ),
                        'timestamp' => array('filter' => FILTER_CALLBACK,
                                             'options' => "validateDate", ), ),
                        'value' => FILTER_VALIDATE_FLOAT, );

The keys in the array args are the same values of the array headers.
How to avoid repeating the values here ?

Here is the answer:

I delete the keys from the args array, and I create a new array from combining both headers and args:

$argsVal = [ array('filter' => FILTER_VALIDATE_REGEXP,'options' => array('regexp' => '/^[a-zA-Z0-9_:.()\/]*$/'), ),
               array('filter' => FILTER_CALLBACK,'options' => "validateDate", ), FILTER_VALIDATE_FLOAT, ];

$args = array_combine($headers,$argsVal);


C# - Linq - Techniques for avoiding repeating same pieces of code I am writing a piece of code for C# Web Api, letting the clients to pass a column name and sort direction as parameter.
Although there are, like, 30-ish properties, so the following code (despite it works) gets ugly after a while.
What are my options to avoid repeating this seemingly same pieces of code?
if (column == nameof(MyModel.ColumnA).ToLower())
{
    if (parameters.IsOrderByAsc)
    {
        return queryResult.OrderBy(q => q.ColumnA);
    }

    return queryResult.OrderByDescending(q => q.ColumnA);
}

if (column == nameof(MyModel.ColumnB).ToLower())
{
    if (parameters.IsOrderByAsc)
    {
        return queryResult.OrderBy(q => q.ColumnB);
    }

    return queryResult.OrderByDescending(q => q.ColumnB);
}

if (column == nameof(MyModel.ColumnC).ToLower())
{
    if (parameters.IsOrderByAsc)
    {
         return queryResult.OrderBy(q => q.ColumnC);
    }

    return queryResult.OrderByDescending(q => q.ColumnC);
}

....

```

If you're willing to use the DynamicLinq library, the problem becomes trivial. It allows you to pass a string parameter to the OrderBy method, bypassing your entire logic:
queryResult.OrderBy(column)

If this is related to doing a SQL database lookup, if you think about it, the issue you're facing it a bit of a self-imposed issue, because you start from a string value and you're eventually going to generate a string value (in the SQL query). The issue you're facing is that standard LINQ forces you to map it to an actual property of the entity, which in this case is an unnecessary mapping which DynamicLinq allows you to bypass.
If you want, you could add a bit of validation to confirm that the passed string is a known property of your entity type.
bool columnIsProperty = typeof(MyModel)
                           .GetProperties()
                           .Any(prop => prop.Name.ToLower() == column.ToLower());

if(columnIsProperty)
    queryResult = queryResult.OrderBy(column);


Improving a priority queue sketch in Racket/Scheme I just put together the skeletons for a priority queue using a binary heap in racket/scheme. Using racket/scheme is all about the educational experience and I was wondering if anyone  wants to critique and improve the code. Bug reports are welcome

;; priority queues operations on integer values

(define (heap)
  (let ((the-heap (make-vector 10)) (next-free 1))

(define (swim val)
  (begin
    (vector-set! the-heap next-free val)
    (set! next-free (+ next-free 1))
    (let loop ((i (- next-free 1)))
      (if (and (< (vector-ref the-heap (quotient i 2)) (vector-ref the-heap i)) (> (quotient i 2) 0))
          (begin
            (exch the-heap i (floor (/ i 2)))
            (loop (floor (/ i 2))))
        the-heap))))

(define (sink index)
  ;; a call to this method is used to re-establish order within the
  ;; heap by 'sinking' the value rooted at index to its right position
  ;; within the whole heap
  (let loop ((node index) (kid (* 2 index)))
    (if (<= kid next-free)
        (let ((larger-kid (if (> (vector-ref the-heap kid) (vector-ref the-heap (+ kid 1)))
                                 kid
                                 (+ kid 1))))
          (if (> (vector-ref the-heap larger-kid) (vector-ref the-heap node))
              (begin
                (exch the-heap node larger-kid)
                (loop larger-kid (* 2 larger-kid)))
              the-heap))
          (newline))))

(define (delete-max)
  ;; delete and return the maximum value in the heap.
  ;; add the last item on the heap to the root position
  ;; sink to re-establish order within the heap of the heap and call
  ;;
  (begin
    (let ((val (vector-ref the-heap 1)))
      (vector-set! the-heap 1 (vector-ref the-heap (- next-free 1)))
      (set! next-free (- next-free 1))
      (vector-set! the-heap next-free 0)
      (sink 1)
      val)))

(define (put value)
  (swim value))

(define (size)
  (vector-length the-heap))

(define (empty?)
  (= next-free 1))

(define (exch vec lo j)
  (let ((lo-val (vector-ref vec lo)) (j-val (vector-ref vec j)))
    (begin
      (vector-set! vec lo j-val)
      (vector-set! vec j lo-val))))

(define (dispatch message . arg)
  (cond
    ((eq? message 'delete-max) (delete-max))
    ((eq? message 'put) (put (car arg)))
    ((eq? message 'size) size)
    ((eq? message 'heap) the-heap)
    ((eq? message 'empty?) empty?)))
dispatch))

Here are some general points before I start on more specific points:

I don't see any unit tests. Racket programmers often write many unit tests for each function. You can use the module+ construct and rackunit to easily write internal unit tests.
Indentation is important for clarity. I couldn't tell that everything was internal to heap at first because of the lack of indentation. Also, you can replace some of the lets with internal defines and make some of the internal functions into top-level functions to reduce right-ward drift.

More specific points:

It looks like you're using a dispatch function with internal state to keep track of the priority queue state. The more natural thing to do in Racket would be to create a new datatype using struct. This way, your implementation details don't leak through.
You use a dispatch function as the entry point to your simulated object. Simulating OO style in this fashion is not a very natural thing to do in Racket. Racket programmers tend to either use (1) the built-in classes & objects or (2) have a functional API with a function for each operation.
You use begin to sequence multiple effecting operations inside functions. These are actually unnecessary since Racket functions can have as many expressions in the body as you like.

Hope that helps. Cheers.


Loading configuration properties from XML I would like a review of the transformation of the first method to the updated one below:

 private static ArrayList<String> getValuesfromXML(String key1, String key2) {
    ArrayList<String> valueArray = new ArrayList<String>();
    try {

        String scriptPath = getScriptPath("prop.xml");
        File file = new File(scriptPath);
        FileInputStream fileInput = new FileInputStream(file);
        Properties properties = new Properties();
        properties.loadFromXML(fileInput);
        fileInput.close();
        valueArray.add(properties.getProperty(key1));
        valueArray.add(properties.getProperty(key2));

    } catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }

    return valueArray;
}

It looks as if it's GC'd so there's no memory leaks. And is not using a finally on the filehandle really an issue?

    /**
     * read property values from the configuration file. Using standard Java Properties class.
     *
     * @param key1
     *            - Property key
     * @param key2
     *            - Property Key
     * @return - ArrayList values of properties based on keys
     */

    private static ArrayList<String> getPropertyValue (String key1, String key2){

        final ArrayList<String> valueArray = new ArrayList<>();
        String scriptPath = getScriptPath(propertiesFileName);

        try (FileInputStream fileInput = new FileInputStream(scriptPath)) {
            Properties properties = new Properties();
            properties.loadFromXML(fileInput);
            valueArray.add(properties.getProperty(key1));
            valueArray.add(properties.getProperty(key2));

        } catch (Exception  ex) {
            ex.printStackTrace();
        }

        return valueList;
    }

Your conversion is pretty good. I would make some suggestions but on the whole it would "pass" a code review - depending on the code that's calling this function (yes, that does make a difference). If I was to suggest changes they would be...

Interfaces over concrete classes.

Use interfaces instead of concrete types for return values and parameters. In your case, your function returns ArrayList<String>, but I would convert that to List<String>. This may not be possible without changing other code in your system, though. That would also imply changing the variable final ArrayList<String> valueArray to be final List<String> valueArray.

Error Handling

Your handling sucks.... but if you're locked in to the function signature because of other code, then you can't do much to change it, easily. Still, printing the stack trace is a very poor-man's solution.

Note that you can use a multi-exception catch now, and do:

} catch (IOException | FileNotFoundException e) {
    // ... handle e
}

Use java.nio.file.*

The newer NIO functions typically are more efficient, so use them when you can.

In your case, I would have:

Path script = Paths.get(getScriptPath(propertiesFileName));
try (InputStream input = new BufferedInputStream(Files.newInputStream(scritpt))) {
    // .....
} catch (.....) {
    // .....
}

Note that the input is an InputStream and not a FileInputStream ... use the highest form of the instance where you can.

Declare variables where they are used....

You declare the result variable valueArray outside the try-block. This is because of how you do the error handling and you may want to return it when it has an empty value. This is not the best solution though, I would have:

try (InputStream input = new BufferedInputStream(Files.newInputStream(scritpt))) {
    // .... get properties
    List<String> valueArray = new ArrayList<>();
    // .... populate it....
    return valueArray;
} catch (....) {
    // ... report the error
    e.printStackTrace();
    // ... return an empty array.
    return Collections.emptyList();
}

The finally....

You are using a finally on the file handle close... that's what the try-with-resources does for you.


Given an integer s and a list of integers ints finding (x,y) in ints such that s = x + y The following function sum_pairs(ints, s) takes two inputs: ints is a list with integer values and s is a integer number. The function sum_pairs returns the values (x, y) in ints such that \$s = x + y\$ and the index of y is the lowest for the (x, y) in ints such that \$s = x + y\$. This is a problem taken from codewars. The function passes all the test cases. However, when I attempt to run the code for a big list, the server codewars gives me the following answer: "Process was terminated. It took longer than 12000ms to complete".

I am wondering whether the code I have written to solve the problem can be optimised or should I use a different approach to give a solution.

def sum_pairs(ints, s):

    begin = 1
    end = len(ints)
    memo = []

    while begin < end:
        try:
            memo.append([begin-1, ints.index(s - ints[begin-1], begin, end)])
            begin += 1
            end = memo[-1][1]
        except:
            begin +=1

    if memo == []:
        return None
    else:
        return [ints[memo[-1][0]], ints[memo[-1][1]]]

The function sum_pairs(ints, s) searches for \$y = s - x\$ in the list ints using the function index(). If a value of y is found the index for the pair (x,y) is added to memo, then the range for the search (begin, end) are updated. Else only the lower limit begin for the search is updated.

Your code's complexity is \$O(n^2)\$, this means that worst case it's on-par with looping through ints in a nested loop, and returning the maximum for x, or the minimum for y. And so it's on-par with:

def sum_pairs(ints, s):
    try:
        return max(
            (x, y)
            for x in ints
            for y in ints
            if x + y = s
        )
    except ValueError:
        return None

You could speed this up, if ints is sorted, which your code implies, then you could change max to next, and flip the x and y. However, this still has \$O(n^2)\$ time.

The simplest way to force \$O(n)\$ time is to use a deque, and consume both ends of the sorted deque. This works by removing the tail of the deque to the point that \$x + y \le s\$. After this if \$x + y = s\$, then we return that. If not we take the next item at the head of the deque. Something like:

from collections import deque

def sum_pairs(ints, s):
    q = deque(ints)
    try:
        while True:
            y = q.popleft()
            n = s - y
            while q[-1] > n:
                q.pop()
            if q[-1] == n:
                return q[-1], y
    except IndexError:
        return None


correctGuess() and incorrectGuess() functions for a quiz In my JavaScript quiz, I have two functions below which are not DRY. What i want to do is to cut it, so everything below var = text would be used only once not twice.

My concept is to enclose these two functions in bigger function (e.g. guess()) and keep the trimmed correctGuess() and incorrectGuess() within it.

Now here's the question: how can I call such nested function as described above from outside scope. I was thinking about something like: guess().correctGuess() which is obviously wrong but I wanted to share a concept.

Additionally, when e.g. correctGuess() would be called, is rest of the commands within our main guess() function would be executed?

Fiddle with full code

function correctGuess(i) {

    totalScore++;
    questionNumber++;

    var text = "Correct!";

    var updatePage = ['
' +
        '
' + text + '
' +
        '
Total Score: ' + totalScore + '

'
    ];

    mainContent[html](updatePage);

    $('#answerDiv')[fadeIn]("slow");
    $('#answerDiv').append('<button id="nextButton">Next Question</button>');

    $('#nextButton').on('click', function() {
        if (questionNumber == allQuestions.length && totalScore <= 4) {
            results()
        } else {
            question(questionNumber)
        }
    })
};

function incorrectGuess(i) {
    totalScore--;
    questionNumber++;

    var text = "Wrong!";

    var updatePage = ['
' +
        '
' + text + '
' +
        '
Total Score: ' + totalScore + '

'
    ];

    mainContent[html](updatePage);

    $('#answerDiv')[fadeIn]("slow");
    $('#answerDiv').append('<button id="nextButton">Next Question</button>');

    $('#nextButton').on('click', function() {
        if (questionNumber == allQuestions.length && totalScore <= 4) {
            results();
        } else {
            question(questionNumber);
        }

    });

};

i did not test or execute this code

just put it in an object?

var Guess = {

    score : 0,
    questions_count : 0,

    text : null,

    correct : function () {
        this.score+= 1;
        this.questions_count += 1;
        this.text = 'Correct!';
    },

    incorrect : function () {
        this.score-=1;
        this.questions_count += 1;
        this.text = 'Wrong!';
    },

    update : function () {
        var updatePage = ['
' +
        '
' + text + '
' +
        '
Total Score: ' + this.score + '

'
        ];

        mainContent[html](updatePage);

        $('#answerDiv')[fadeIn]("slow");
        $('#answerDiv').append('<button id="nextButton">Next Question</button>');

        $('#nextButton').on('click', function() {
            if (this.questions_count == allQuestions.length && totalScore <= 4) {
                results();
            } else {
                question(this.questions_count);
            }

        });
    }
};

chaining

key to chaining calls is to return the right thing. Simple example:

function test() {
    var x = {
        correct : function () {
            console.log('correct');
        },
        incorrect : function() {
            console.log('incorrect');
        }
    };

return x;
}

test().correct();


Understanding polymorphism in Python I feel like I don't understand how polymorphism works in Python. Here is something I put together.

class _Tag(object):
    def __init__(self):
        #DisjointedTag and JoinedTag will bypass __init__.
        #Also, self will not be defined within this class.
        #Bottom line, this class is just for inheritance.
        raise NotImplementedError;

    def join_attributes(self):
        self.joined_pieces=''.join(self.funct_pieces);
        self.__class__=JoinedTag;

    def to_disjointed(self):
        self.__class__=DisjointedTag;

    def add_attribute(self, attribute, value):
        self.funct_pieces.insert(
            -1, ' {}="{}"'.format(attribute, value)
        );
        self.to_disjointed();

class Tag(_Tag):
    def __init__(self, name):
        self.funct_pieces=[
            i.format(name) for i in ['<{0}', '>{{}}</{0}>']
        ];
        self.joined_pieces='';

    def __call__(self, contents):
        self.join_attributes();
        return self(contents);

class JoinedTag(_Tag):
    def __call__(self, contents):
        return self.joined_pieces.format(contents);

class DisjointedTag(_Tag):
    #JoinedTag.__call__ has 1 mandatory parameter so this also needs one.
    def __call__(self, place_holder):
        raise TypeError(
            '{} is disjointed because attributes were added to it.'.\
            format(self)
        );

Here is an example:

>>> p=Tag('p')
>>> p('hi')
'
hi
'
>>> p.add_attributes('align', 'left')
>>> p.join_attributes();
>>> p('hi')
'
hi
'

I could have joined the attributes on every call but that would waste time. The code I made has no if statements, so it should be more readable according to the philosophy of polymorphism. However, it is very hard to read. I figure I've done something wrong or I'm not used to reading polymorphic code.

I'm a bear of very little brain, so I don't really understand what you are trying to achieve, or why you think polymorphism will help you achieve it.

But I did notice some problems with your code:

There's no documentation.
There are no test cases.
Python doesn't need semicolons at the end of lines.
You don't need a \ at the end of a line if the statement cannot end at that point because of an unclosed parenthesis.
There are no checks on the validity of entity and attribute names.
Values aren't escaped.
There's no check to stop the caller adding multiple attributes with the same name.
You shouldn't raise an exception class, you should raise an instance of that class (passing a suitable error message to the constructor).


Remove Nth Node from End of Linked List I am solving the well known problem Remove Nth Node From End of List:

  Given a linked list, remove the n-th node from the end of list and
  return its head. Assume that n is between 0 and the length of the list.

  Example:

Given:  1->2->3->4->5, and n = 2
Return: 1->2->3->5.

This is my solution:

public ListNode removeNthFromEnd(ListNode head, int n) {
        if(head == null || n == 0)
            return head;
        ListNode slow = head;
        ListNode fast = head;
        ListNode newHead = new ListNode(-1);
        newHead.next = head;
        ListNode pre = newHead;
        int steps = 1;
        while(steps <= n){
            fast = fast.next;
            steps++;
        }
        if(fast == null)
            return head.next;
        while(fast.next != null){
            pre = pre.next;
            slow = slow.next;
            fast = fast.next;
        }
        if(slow.next != null)
            slow.next = slow.next.next;
        else // slow.next = null, i.e. I have to remove the last node
            pre.next = null;
        return head;
    }

and this is the definition of ListNode class:

public class ListNode {
      int val;
      ListNode next;
      ListNode(int x) {
          val = x;
          next = null;
      }
  }

What I don't like in my code is that I handle every edge case separately. Is there more sophisticated way to deal with the edge cases? Also the following code fragment:

       while(steps <= n){
            fast = fast.next;
            steps++;
        }

Is this the right condition for traversing the list? This definitely works, but is it better to rewrite my code in terms of

      while(steps < n){
            fast = fast.next;
            steps++;
      }

Bug

I cannot remove an element from a one-element list. You could easily fix this by adding this after your initial check:

if(head.next == null && n == 0) {
    // return empty list
}

ListNode and the need for a List

The questions starts with Given a linked list. But you don't really have a linked list. You have a node class, but that's it, and that's really not enough. Right now, I would have to take your word that it does what it is supposed to, because there is no way of knowing if it actually does (well, except reading the code :) ).

Creating something like a list, I would have to do this:

    ListNode n = new ListNode(1);
    n.next = new ListNode(2);
    n.next.next = new ListNode(2);

And if I want to write unit tests as @Pimgd suggested (or even just check with a simple example in a main method), I would need to add a whole lot of code. I would suggest that you at least add methods to add elements, to traverse through the list, to check if one list is equal to another, and a toString method.

This would make debugging and testing your code a lot easier, and some of those methods would also help you implement the removeNthFromEnd method.

Handling Exceptions

Don't just accept any input. You are already checking if(head == null || n < 0), which is good, but it would be best to throw an IllegalArgumentException.

If the user tries to remove an element that doesn't exist, don't just let them, throw an IndexOutOfBoundsException instead.

while loop

    while(steps <= n){
        fast = fast.next;
        steps++;
    }

Your first while loop looks better as a for loop:

for (int steps = 1; steps <= n; steps++) {
    fast = fast.next;
}

As to your question, it doesn't really matter if you start at 1 or 0. I would start at 0, but it's just a personal preference.

Naming and Comments

I just want to emphasize what @Pimgd and @shivsky said. You really need better names than fast and slow, and if the program isn't a lot clearer then, also comments (see this code for an example; you don't need that many comments, but something like set p1 and p2 apart by n-1 nodes initially etc is really helpful).

You should also have a method comment. It should state if the list is changed or if a new (changed) list is returned (or what else is returned if not a new list), what arguments are acceptable, etc.

You should also make clear how your indices work. Does nth from last mean that I remove the last node with removeNthFromEnd(0) or removeNthFromEnd(1)?

Misc

declare your function as static to express how it works (it doesn't belong to a specific ListNode and changes that).
use curly brackets even for one-line statements. If you really don't want to, put the line on the same line as the if statement. Otherwise it's easy to introduce bugs.


Splitting a list into overlapping sub-lists I'm very new to Haskell. I did try it for a few days, a few years ago... but other than that, this is my second day. I'm equally unused to functional programming, by the way.

After much trial and error, I managed to write this function:

chunk :: Int -> [a] -> [[a]]
chunk n xs = chunk' n [] xs
    where
    chunk' _ results [] = results
    chunk' n results xs
        | length xs >= n = chunk' n (results ++ [take n xs]) $ tail xs
        | otherwise = chunk' n results []

This creates a list of all possible length n sub-lists, like so:

*Main> chunk 3 [1..7]
[[1,2,3],[2,3,4],[3,4,5],[4,5,6],[5,6,7]]
*Main> chunk 5 [1..7]
[[1,2,3,4,5],[2,3,4,5,6],[3,4,5,6,7]]

How can this be improved (in readability, performance, brevity etc.)?

Using ++ to concatenate lists is a bad idea for performance, and it's a sign that you are not using recursion smartly.

Here's what I came up with:

chunk :: Int -> [a] -> [[a]]
chunk n xs =
  if   length chunk' < n then []
  else (chunk' : chunk n (tail xs))
    where chunk' = take n xs


Non-re-indexing List with defragmentation I'm currently coding something where I have Objects that take on an ID, and are stored in a List. For easy retrieval, the ID of the object is it's index in the list. However, that means that the object cannot be removed from the list without everything going to hell, since the list will be re-indexed and thus I won't be able to retrieve the right object from the list by knowing its ID anymore.

I also did not want to use a Map, since that just looks ugly and makes a big part of my code look more complicated than it should.

Thus, I tried to simply use a list, but modify it in order to be able to get an index that won't change as long as the object is alive.

@SuppressWarnings("serial")
public class SafeIndexList<E> extends ArrayList<E> {

    Stack<Integer> safeIndexes = new Stack<Integer>();

    @Override
    public E remove(int index){
        if(this.size()-1 == index) return super.remove(index);
        else {
            E e = this.get(index);
            this.set(index, null);
            safeIndexes.push(index);
            return e;
        }
    }

    public int getSafeIndex(){
        return safeIndexes.size() > 0 ? safeIndexes.pop() : size();
    }

}

Is this "good" or just complete, sorry for the wording, bullshit?

It's not complete BS. Your use-case is somewhat surprising, and your disregard for using a Map is also surprising, but given those, you're left with little option other than a completely custom implementation.... but, let's look at the issues you currently have...

People need to do a round-trip query-before-inserting operation - they need to call getSafeIndex() before setting a new value.

People can't use all the methods available to them. For example, they can't call:

clear() (See code)
add(int, obj) (See code)
..... none of the iterator, or stream-related methods (foreach, etc.) because they will return null values mixed in the wrong places.
size() will lie to them.

... in fact, the only methods that can be used reliably are set(...), get(...), and similar ones.

Extending ArrayList seems to introduce a lot of ways to break the code/data integrity.

I recommend encapsulation instead. In fact, this is a common "debate": Inheritance vs. Composition/Encapsulation (search for those topics on google).

In this case, composition/encapsulation would remove a lot of your issues, and reduce the class to just what you need.

Underlying your class you could still have the ArrayList and Stack, (except Stack class is deprecated in favour of Deque interfaces) but it would look something like:

@SuppressWarnings("serial")
public class SafeIndexList<E> {

    List<E> data = new ArrayList<>();
    Deque<Integer> safeIndexes = new ArrayDeque<>();

    public E remove(int index){
        if(index < data.size())
            // check double-removes.
            if (!safeIndexes.contains(index)) {
                safeIndexes.push(index);
            }
            return super.remove(index);
        }
        return null;
    }

    public int add(E value){
        int idx = safeIndexes.isEmpty() ? data.size() : safeIndexes.pop();
        data.set(idx, value);
        return idx;
    }

}

You can add methods for other features you need, as needed. Note, that having abstracted/encapsulated the underlying data store mechanism, you can easily replace the array/deque with a Map now, and not notice.

Edit: to add in the combined constructor/index problem, a solution like this may help, adding the method:

    public int construct(IntFunction<E> factory) {
        int idx = safeIndexes.isEmpty() ? data.size() : safeIndexes.pop();
        data.set(idx, factory.apply(idx));
        return idx;
    }

Now, you can call that with something like:

int id = mylist.construct(i -> new MyObject(..., i, ...));

The above constructs the inner object with the index, but "internal" to the list too. Alternatively, you can return the constructed object if that makes more sense....

MyObject obj = mylist.construct(i -> new MyObject(..., i, ...));


Random weighted 'tick' function This is a very simple function, but it gets run on a roughly 10ms interval, so I'm looking for ways to improve it. The first obvious thing is that it has to run the comparisons nearly every time as it starts with the lowest probabilities first. Is there a way I can reverse that without breaking it, or a way I can generally improve the performance of this function to achieve the same end result?

function find_coin() {
    var roll = Math.random() * 100;

    if (roll < 0.01)
        currency['platinum']++;
    else if (roll < 0.1)
        currency['gold']++;
    else if (roll < 1)
        currency['silver']++;
    else if (roll < 10)
        currency['bronze']++;
};

Reversing the order of the tests is probably your best option here. In your current code:

function find_coin() {
    var roll = Math.random() * 100;

    if (roll < 0.01)
        currency['platinum']++;
    else if (roll < 0.1)
        currency['gold']++;
    else if (roll < 1)
        currency['silver']++;
    else if (roll < 10)
        currency['bronze']++;
};

90% of your random values will be >= 10, additionally, 9 % will be between 1 and 10 %, so, 99% of your tests will have to check.... 4 roll conditions (< 0.01, < 0.1, < 1, and < 10). So, 99% of all your rolls will check 100% of your conditions.

If you reverse the logic, and do:

function find_coin() {
    var roll = Math.random() * 100;

    if (roll >= 10) {
        return
    }

    if (roll >= 1) {
        currency['bronze']++;
    } else if (roll >= 0.1) {
        currency['silver']++;
    } else if (roll >= 0.01) {
        currency['gold']++;
    } else {
        currency['platinum']++;
    }

};

With the above code, 90% of your rolls will do just one test, 9% will do 2 tests, 0.9 will do 3 tests, and so on.

Note, also, that I have used braces even for the 1-liner blocks in the conditions. This is a good practice and it avoids bugs when maintaining your code, later.


Script to encrypt a file and add it to IPFS(ipfs.io) Here is the script that I would like reviewed for the following:

Best practices and design pattern usage
Correctness in unanticipated cases

The script is made to store (IPFS) Interplanetary File System hash objects. If you would like to know more check out https://ipfs.io

3rd party packages:

ipfsapi PyPi page
gnupg PyPi page

Script:

#!/usr/local/bin/python3

import os
import gnupg
import ipfsapi

# Set GPG Home directory
gpg = gnupg.GPG(homedir='')
fileToEncrypt = input("Full path to file to encrypted and posted \n:")
encryptedFile = (fileToEncrypt + ".gpg")

def encryptFileFunction():
    passphrase = input("Password you would like encrypt with \n:")
    with open(fileToEncrypt, 'rb') as f:
        status = gpg.encrypt(f,
        encrypt=False,
        symmetric='AES256',
        passphrase=passphrase,
        armor=False,
        output=fileToEncrypt + ".gpg")

def ipfsFileFunction(encryptedFile):
    # Tell module where IPFS instance is located
    api = ipfsapi.connect('127.0.0.1', 5001)
    # Add Encrypted file to IPFS
    ipfsLoadedFile = api.add(encryptedFile)
    # Return Hash of new IPFS File
    ipfsHash = (ipfsLoadedFile['Hash'])
    return ipfsHash

def delEncryptedFile(encryptedFile):
    if os.path.isfile(encryptedFile):
        os.remove(encryptedFile)
    else:
        print("Error: %s file not found" % encryptedFile)

def main():
    encryptFileFunction()
    ipfsFileFunction(encryptedFile)
    print ("File encrypted and added to IPFS with this hash " +  ipfsFileFunction(encryptedFile))
    delEncryptedFile(encryptedFile)

main()

Some things:

You'll want to wrap the call to main in if __name__ == '__main__': to be able to import from your package in other packages.
For maximum flexibility use argparse to pass the file name rather than making an interactive script.
Format strings are generally preferred over string concatenation, so fileToEncrypt + ".gpg" becomes "{}.gpg".format(fileToEncrypt).
Run the code through pycodestyle to get a bunch of tips on writing idiomatic Python. For example, Python appropriately uses snake_case for variable, function and method names.
Function in a function name is completely redundant. Use a highlighting editor or (ideally) an IDE to trivially distinguish between functions and variables.
Magic strings like encryption parameters and the IPFS address should be moved to either a configuration file, a command line parameter (possibly with a default) or a constant at the top of the file.
The shebang line should be #!/usr/bin/env python3 for maximum portability - it basically means that the first python3 executable on the PATH will be used.
You have a race condition in delEncryptedFile. Rather than checking for a file and then deleting it you should just try to delete it and then let the exception bubble up to the user if it fails. If at some point you want a nicer formatted error message that should be generated in main, not further down, so that, again, other packages can use your package easily.


Is this a fair use for a factory pattern? class AcmeFactory
{
   public static function anvil()
   {
      $crud           = new Crud;
      $productTypes   = new ProductTypes($crud);
      $productImages  = new ProductImages($crud);
      $product        = new Anvils ($crud, $productImages);
      $productManager = new ProductManager ($productTypes, $product);

      return $productManager;
   }

   public static function jetPack()
   {
      $crud           = new Crud;
      $productTypes   = new ProductTypes($crud);
      $productImages  = new ProductImages($crud);
      $product        = new JetPacks ($crud, $productImages);
      $productManager = new ProductManager ($productTypes, $product);

      return $productManager;
   }
}

I know the above code works, but am I using the factory pattern correctly?

The intended result is this:

$anvil = new AcmeFactory::anvil();
$anvil->create($weight);
$anvil->display($id);
.....

Rather than doing it that way (separate methods for separate configurations) you should pass in the "configuration" to a single method and let that one handle it.

class AcmeFactory
{
    public static function create($config)
    {
        $crud           = new Crud;
        $productTypes   = new ProductTypes($crud);
        $productImages  = new ProductImages($crud);
        $product        = null;
        if($config == "anvil")
        {
            $product = new Anvils($crud, $productImages);
        }
        else if($config == "jetpack")
        {
            $product = new JetPacks($crud, $productImages);
        }
        $productManager = new ProductManager($productTypes, $product);

        return $productManager;
    }
}

You can of course extract the choice of product into a separate (private) function again ;)

then you'd use this as follows:

$anvil = AcmeFactory::create("anvil");
$anvil->create($weight);
$anvil->display($id);
...

I am not that good with PHP, but I think you should somehow limit what you allow to be passed into your create function. In Java I'd solve this using an enum, but as I said, I don't know enough php :(

Apart from that, I have a minor nitpick too:

Be consistent in your use of whitespaces before parentheses. Compare:

$productImages  = new ProductImages($crud);
$product        = new Anvils ($crud, $productImages);


Quicksort Algorithm Efficiency I've just tried my hand at Quicksort, and I seem to have gotten it working (thanks to Stack Overflow). However, is it "true" Quicksort? And is it decently written?

public static void sort(List<Integer> arr, int left, int right) {
        int i = left - 1;
        int j = right + 1;
        if (right - i == 0 || right - i == 1) return;
        int v = arr.get(right);

        for(;;) {
            while(arr.get(++i) < v);
            while(v < arr.get(--j) && j != 0)
                if(j == 1)break;
            if(i >= j)break;

            Collections.swap(arr, i, j);
        }
        Collections.swap(arr, i, right);

        sort(arr, left, i - 1);
        sort(arr, i, right);
}

I know I should use a regular array (int[]) for speed, but we can ignore that for now.

int v = arr.get(right);

I assume v holds the partitioned item. But I don't know what you mean by v, so you can make your variable names more clear. Java's convention is to use CamelCase for variable names, so you might want to do that.

   if (right - i == 0 || right - i == 1) return;

As I understand it, if right - i is either 0 or 1, the method will return. So why not just check if right - i is less than or equal 1?

    if (right - i <= 1) return;

It might be more readable to put your statements inside braces, I think:

        while(arr.get(++i) < v);
        while(v < arr.get(--j) && j != 0)

might be confusing: Perhaps it would be more understandable if you do this:

        while(v < arr.get(--j) && j != 0) {
            if(j == 1)break;
        }

Regarding that code, the condition j != 0 will never be false because the loop will stop when j becomes 1. I think you can eliminate the if statement like this:

        while(v < arr.get(--j) && j > 1);

Currently your method signature is:

public static void sort(List<Integer> arr, int left, int right)

If we want to sort all items, we might call it as:

sort(list, 0, list.size()-1);

It might be better to write an overloaded method for convenience, Like this:

public static void sort(List<Integer> arr) {
    sort(arr, 0, arr.size()-1);
}

Now if someone wants to sort all items in the list, they don't have to provide the beginning and end of the sort.


Skip input to next line, for reading serialized objects I was de-serializing objects that had an extra newline that needed to be removed before getting the next object.

#ifndef THORSANVIL_UTIL_H
#define THORSANVIL_UTIL_H

// Removed headers for   getDist() and getRandomContainerIterator()
// We already reviewed those two.
#include <iostream>

namespace ThorsAnvil
{
    namespace Util
    {

// Removed getDist() and getRandomContainerIterator()
// We already reviewed those two.

class IgnoreUntilNewLine
{
    public:
        friend std::istream& operator>>(std::istream& stream, IgnoreUntilNewLine& /*data*/)
        {
            stream.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
            return stream;
        }
};

class SkipEmptyLineToEnd
{
    public:
        friend std::istream& operator>>(std::istream& stream, SkipEmptyLineToEnd& /*data*/)
        {
            std::string     emptyLine;
            std::getline(stream, emptyLine);
            if (!emptyLine.empty()) {
                stream.setstate(std::ios::badbit);
            }
            return stream;
        }
};

    }
}

#endif

Simplified Example for usage:

namespace TU = ThorsAnvil::Util;
struct Example1
{
     int    x;
     int    y;
     std::string  line; // Always one line with no "\n"
     void swap(Example1& other) noexcept {/*Stuff*/}

     friend std::ostream& operator<<(std::ostream& stream, Example1 const& data)
     {
         // Note putting a new line here
         // to make the Serialized object more readable.
         // The actual object was a bit more complicated and
         // I really wanted to make it readable.
         return stream << data.x << " " << data.y << "\n"
                       << data.line << "\n";
     }
     friend std::istream& operator>>(std::istream& stream, Example1& data)
     {
         Example1  tmp;
         if (stream >> tmp.x >> tmp.y)
         {
             stream.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
             if (std::getline(stream, tmp.line)) {
                 data.swap(tmp);
             }
         }
         return stream;
     }
};

I thought the above read was a bit cumbersome and hard to read. Thus the two types above. This means I can re-write the read operation as:

     friend std::istream& operator>>(std::istream& stream, Example1& data)
     {
         TU::IgnoreUntilNewLine ignoreNewLine;
         Example1               tmp;

         if ((stream >> tmp.x >> tmp.y >> ignoreNewLine) && std::getline(stream, tmp.line)) {
             data.swap(tmp);
         }
         return stream;
     }

IgnoreUntilNewLine is too long in my opinion.  I'd simplify it to ignore_line.

It probably makes sense to provide the manipulator object in the library instead of letting the user declare one:

struct ignore_line_t {
    constexpr explicit ignore_until_newline_t() = default;
    // ...
};
inline constexpr ignore_line_t ignore_line{};

so the user can use it directly in a >> chain:

stream >> tmp.x >> tmp.y >> TU::ignore_line

Incidentally, we can go further and provide a similar helper for getline to fit in the chain:

class getline {
    std::string& dest;
public:
    explicit getline(std::string& s)
        : dest{s}
    {
    }
    friend std::istream& operator>>(std::istream& is, getline man)
    {
        return std::getline(is, man.dest);
    }
};

so your example can be written simply as

stream >> tmp.x >> tmp.y >> TU::ignore_line >> TU::getline(tmp.line)


Implementing faster StringBuilder that only works with ASCII strings I wrote a StringBuilder that works only with ASCII strings. the current append performance is almost identical to BCL StringBuilder append performance. I want some tips on how to make it faster, and general tips about the code. I did this for educational purposes.

Implementation:

public sealed class FastStringBuilder
{
    private byte[] chars;

    public int Length { get; private set; }

    public FastStringBuilder(int capacity)
    {
        chars = new byte[capacity];
    }

    public FastStringBuilder()
    {
        chars = new byte[16];
    }

    private void GrowBy(int length)
    {
        var temp = chars;
        chars = new byte[length];
        Array.Copy(temp, 0, chars, 0, Length);
    }

    public unsafe void Append(string str)
    {
        if (str.Length + Length >= chars.Length)
            GrowBy((str.Length + Length) * 4);
        else if(Length == chars.Length)
            GrowBy(chars.Length*2);

        fixed (char* p = str)
        {
            for (int i = 0; i < str.Length; i++)
                chars[Length++] = (byte)(p + i);
        }
    }

    public override string ToString()
    {
        unsafe
        {
            fixed (byte* bp = chars)
            {
                return new string((sbyte*)bp,0,Length);
            }
        }
    }
}

Performance tests spec:

public static void StringBuilderTest()
{
    Stopwatch sw = Stopwatch.StartNew();

    StringBuilder builder = new StringBuilder();

    for (int i = 0; i < 1000000; i++)
    {
        builder.Append("F0000000000000000000000000000000000000000000000000000" );
    }

    var result = builder.ToString();

    Console.WriteLine(sw.Elapsed);
}

Results:

Compiled in release mode.

FastStringBuilder: ~0.36 seconds
StringBuilder: ~0.27 seconds
FasterStringBuilder (ChrisWue's implementation): ~0.17 seconds

EDIT

mjolka spotted that FastStringBuilder had a bug.

this code was causing the bug:

    fixed (char* p = str)
    {
        for (int i = 0; i < str.Length; i++)
            chars[Length++] = (byte)(p + i);
    }

I've fixed it.

public sealed class FastStringBuilder
{
    private byte[] chars;

    public int Length { get; private set; }

    public FastStringBuilder(int capacity)
    {
        chars = new byte[capacity];
    }

    public FastStringBuilder()
    {
        chars = new byte[16];
    }

    private void GrowBy(int length)
    {
        var temp = chars;
        chars = new byte[length];
        Array.Copy(temp, 0, chars, 0, Length);
    }

    public unsafe void Append(string str)
    {
        if (str.Length + Length >= chars.Length)
            GrowBy((str.Length + Length)*4);
        else if (Length == chars.Length)
            GrowBy(chars.Length*2);

        for (int i = 0; i < str.Length; i++)
            chars[Length++] = (byte) str[i];
    }

    public override string ToString()
    {
        unsafe
        {
            fixed (byte* bp = chars)
            {
                return new string((sbyte*) bp, 0, Length);
            }
        }
    }
}

This is very, very buggy. This is the most reliable way I could find to reproduce the bug.

static void Main(string[] args)
{
    var sb = new FastStringBuilder();
    var chars = new[] { 'a', 'b', 'e', 'd' };
    sb.Append(new string(chars));
    Console.WriteLine(sb.ToString());
}

Note how args is not used. Now run the program with different arguments:

$ ./Test
dfhj
$ ./Test abc
OZ?'
$ ./Test abcdef
"-~s


Java Streams: Grouping to a Map I was refactoring an ugly piece of code, and I'm unsure if there is a better way of doing this, and whether I'm overdoing it with streams.

Basically I get a bunch of columns, which should be grouped by the name of the sheet they belong to, so that I can return a map of sheet names to a set of column names.

BTW, I can't change the return type (I know it should be a MultiMap).

@Override
public Map<String, Set<String>> getReferencedColumnNames(String columnName) {
    return _formulaSheetSchema.column(columnName).get().formula()
            .computeReferencedColumns(getSheetName()).stream()
            .collect(groupingBy(
                    ColumnRef::sheetName,
                    LinkedHashMap::new,
                    collectingAndThen(toSet(), set ->
                            set.stream().map(ColumnRef::columnName).collect(toSet()))));
}

I'd format this code slightly differently:

return _formulaSheetSchema.column(columnName).get()
  .formula()
  .computeReferencedColums(getSheetName()).stream()
  .collect(groupingBy(ColumnRef::sheetName,
    LinkedHashMap::new,
    collectingAndThen(toSet(), set -> set.stream()
      .map(ColumnRef::columnName).collect(toSet())));

Also the downstream operation you use here is wasteful, because it collects the set and then restreams it. Instead use the mapping collector:

  .collect(groupingBy(ColumnRef::sheetName,
    mapping(ColumnRef::columnName, toSet()));

Sidenote: I personally prefer using 2-space indents across the board, but this formatting works with 4-space indents just as well...

Additionally the _-prefix for instance variables is something I dislike, but so long as you're consistent about it, that should be fine.

I also dislike that you're not checking whether the column actually exists, before accessing it's formula.


Linked List in Java (Garbage Collection) So I'm implementing a linked list in Java, and so far it looks like this:

public class List <T> {

    private class Node {
        T value;
        Node next;
    }

    private Node node_at(int n) {
        Node seeker = front;
        for (int i = 0; i < n; i++) {
            seeker = seeker.next;
        }
        return seeker;
    }

    Node front;
    int length;

    public int length() {
        return length;
    }

    public void insert(int index, T o) {
        if (index == 0) {
            Node n = new Node();
            n.value = o;
            n.next = node_at(0);
            front = n;
        } else {
            Node n1 = node_at(index-1);
            Node n2 = new Node();
            n2.next = n1.next;
            n1.next = n2;
            n2.value = o;
        }
        length++;
    }
    public void append(T o) {
        Node n = node_at(length);
        n.value = o;
        n.next = new Node();
        length++;
    }
    public void append(List<T> l) {
        node_at(length-1).next = l.front; // specific area of interest
        length += l.length;
    }

    public T at(int n) {
        Node seeker = node_at(n);
        return seeker.value;
    }

    List() {
        length = 0;
        front = new Node();
    }
}

The line that I marked specific area of interest is effectively cutting off a node (the last node in the receiver), leaving it homeless. Is that node going to get garbage collected?

Yes, in theory.  That depends on when the garbage collector decides to run.  (There are lots of great explanations about how finicky it is on Stack Overflow.)  But it will become eligible for garbage collection and is gone for all intents and purposes.  Data becomes eligible for garbage collection when no references to it exist, and, assuming node_at(length-1).next is the only reference to it, the Node then qualifies once you change that reference.


Doubly-linked list program with delete functions I'm starting to learn Java (started with C++), and still getting to get used to the syntax and whatnot. For practice, I made a doubly-linked list program. Is there any way this could be improved at all, or is this okay code?

// Doubly Linked-List Practice
package doublylinkedlist;

public class DoublyLinkedList {

node head, tail;

class node // Creates node class
{
    double num;
    node next;
    node prev;
    public node()
    {
        num = 0;
        next = null;
        prev = null;
    }
    public node (double p)
    {
        num = p;
        next = null;
        prev = null;
    }
}

public void append(double x) // Appends data to list
{
    node p = new node(x);
    if (head == null) // Creates head node if empty list
    {
        head = p;
        tail = p;
        return;
    }

    p.prev = tail; // Skips to end of list and appends
    tail.next = p;
    tail = p;

    return;
}

public void showF() // Displays list forward
{
    for (node q = head; q != null; q = q.next)
        System.out.print(q.num + " ");
    System.out.println();

}

public void showR() // Displays list in reverse
{
    for (node q = tail; q != null; q = q.prev)
        System.out.print(q.num + " ");
    System.out.println();
}

public double findAvg()
{
    double mean = 0, total = 0;
    int i = 0;

    for (node p = head; p != null; p=p.next)
    {
        total += p.num;
         ++i;
    }

    return(mean = total/i);
}

public void delete(double x) // Deletes a single node
{
    node p = new node(x);
    node temp = head;
    node pre, nex;

    if (head.num == p.num) // If head node needs to be removed
    {
        head = head.next;
        return;
    }

    while (temp != null) // If a node in between is to be deleted
    {
        if (p.num == temp.num)
        {
            System.out.println("Node found! Deleting " + x + "...");
            temp.prev.next = temp.next;
            temp.next.prev = temp.prev;
            return;
        }
        else temp = temp.next;
    }

    if (tail.num == p.num) // If tail is to be deleted
    {
        tail = tail.prev;
        return;
    }

}

public void deleteMore(double x) // Removes all nodes of certain key
{
    node temp = head;

    if(head == null) // If list is empty
    {
        System.out.println("Empty list!");
        return;
    }

    while (head != null && head.num > x) // If head node needs to be deleted
    {
        head = head.next;
        head.prev = null;
        temp = head;
    }

    while(temp !=null) // Every remaining occurrence to be removed
    {
        if(temp.num > x)
        {
            if (temp.num == tail.num) // If tail node needs to be removed
            {
                tail = tail.prev;
                tail.next = null;
                temp = tail;
            } else
            {
                temp.prev.next = temp.next;
                temp.next.prev = temp.prev;
            }
        }
        temp = temp.next;
    }
}

public static void main(String[] args) {
    DoublyLinkedList myList = new DoublyLinkedList();
    double[] arr = {600.0, 100.0,  10.0,  15.0,  20.0,  200.0,  30.0,  40.0,  300.0, 350.0, 400.0, 500.0};

    for (int i = 0; i < arr.length; ++i)
        myList.append(arr[i]);

    System.out.println("Here's your list foreward...");
    myList.showF();
    System.out.println("Average = " + myList.findAvg());
    System.out.println("Removing all excess averages...");
    myList.deleteMore(myList.findAvg());
    System.out.println("Here's your revised list");
    myList.showF();
    System.out.println("And here's your list in reverse");
    myList.showR();
    System.out.println("Removing 30, just because I can");
    myList.delete(30.0);
    myList.showF();
}

}

Comments in mostly top-down order.

class node // Creates node class

Classes in Java should begin with an uppercase letter.  Ie) class Node.

As it stands, this class can only be used inside DoublyLinkedList, so it should be declared private.  Ie) private class Node.

As a non-static inner class, it implicitly contains a pointer to the DoublyLinkedList object.  You never use this, nor with your current implementation do you need to use it.  It is just taking up extra space.  You can get rid of it by declaring the class static.  Ie) private static class Node.

public node()
{
    num = 0;
    next = null;
    prev = null;
}

This constructor is never used, so probably should be deleted.

If you did want to keep it, you should use constructor chaining, so you don't have to repeat initializations.  Such as:

public node()
{
    this(0);   // Calls node::node(double) constructor
}

public double findAvg()
{
    double mean = 0, total = 0;
    ...

    return(mean = total/i);
}

The parenthesis are not needed.
Possible division by zero, if called on an empty list.
Assignment to mean is unnecessary.  You can remove the assignment and the mean variable.

public void delete(double x) // Deletes a single node
{
    node p = new node(x);

There is no need to create a node here.  You are looking for a node which contains the value x.  Wrapping x in a node and then needing to refer to p.num in tests added complexity for no gain.

    node temp = head;
    node pre, nex;

pre and nex are unused an should be deleted.

    if (head.num == p.num) // If head node needs to be removed

NullPointerException if the list is empty.

    {
        head = head.next;

What of .prev of the new head?  It still points to the removed node.  But if there was only one node in the list, head is now null, and tail is still pointing to the removed node.

        return;
    }

    while (temp != null) // If a node in between is to be deleted

Actually, this will loop for all the nodes, not just the ones "in between".  You will be testing the head node a second time.  And it will test the last tail node as well.

    {
        if (p.num == temp.num)
        {
            System.out.println("Node found! Deleting " + x + "...");
            temp.prev.next = temp.next;
            temp.next.prev = temp.prev;

Fortunately, if the head node matched, the test at the start of the function would process the removal and return.  Otherwise, temp.prev.next = would raise a NullPointerException.

Unfortunately, if the last node happens to match, temp.next.prev = will raise a NullPointerException.

            return;
        }
        else temp = temp.next;
    }

If we get to this point, the number wasn't in the list, so the next code is pointless.

    if (tail.num == p.num) // If tail is to be deleted
    {
        tail = tail.prev;

But if we did get here, tail.prev.next would still point to the original tail node!

        return;
    }

You might consider raising a NoSuchElementException if you never found the value to remove.

}

Many similar comments for deleteMore().

After deleting 1 or more head nodes, you never set the new head.prev to null, or tail to null if you actually ended up deleting all the nodes.

if (temp.num == tail.num) // If tail node needs to be removed

This does not test for reaching the end of the list.  If the list has nodes in the middle which happen to match the value at the end, and that value is above the removal threshold, when you get to that middle node, your processing will skip to the tail node.

To test for the last node, you want

if (temp == tail) // If tail node needs to be removed

or

if (temp.next == null) // If tail node needs to be removed

You should add more test cases, to cover the bug found above.  Once you can show the code breaks, you can fix the code, and demonstrate it works because the test cases now pass.