"Fun with Rotation" - Codechef September Challenge Can you please help me in reducing the complexity of the below code?  The problem statement can be found here.

  You are given an array A of N integers. You are to fulfill M queries.
  Each query has one of the following three types:

  C d : Rotate the array A clockwise by d units.
  A d : Rotate the array A anticlockwise by d units.
  R d : Query for the value of the element, currently being the d-th in the array A.

  Input

  The first line contains two numbers - N and M respectively. The next
  line contains N space separated Integers, denoting the array A. Each
  of the following M lines contains a query in the one of the forms
  described above.

  Output

  For each query of type R output the answer on a separate line.

  Constraints

  1 ≤ N ≤ 100000
  1 ≤ M ≤ 100000
  1 ≤ d ≤ N, in all the queries
  1 ≤ elements of A ≤ 1000000

  The array A and the queries of the type R are 1-based.

The solution in Java manages to solve the problem but is facing a time limit exceeded issue, so please advise me on how to optimize the code.

import java.io.BufferedReader;

 import java.io.IOException;

 import java.io.InputStreamReader;

 import java.util.ArrayList;

 public class Main {

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

    {

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

        String not[]=in.readLine().trim().split(" ");

        int n=Integer.parseInt(not[0]);

        int q=Integer.parseInt(not[1]);

        String s[]=in.readLine().trim().split(" ");

    ArrayList<Integer>  sarray=new ArrayList<Integer>();

        for(int i=0;i<n;i++)

        {

        sarray.add(Integer.parseInt(s[i]));

        }

        for(int j=0;j<q;j++)

        {

            String r[]=in.readLine().trim().split(" ");

            if(!r[0].equals("R"))
            sarray=swap(sarray,n,Integer.parseInt(r[1]),r[0]);

            else

                System.out.println(sarray.get(Integer.parseInt(r[1])-1));

            }

    }

    private static ArrayList<Integer> swap(ArrayList<Integer> arr, int n, int d,String c) {

        // TODO Auto-generated method stub

        int i, j;
        int temp;

        for(j = 0; j < d; j++)

        {

            //rotate right once

            if(c.equals("A"))

            {

            temp = arr.get(n - 1);

            for(i = n - 1; i > 0; i--)

            arr.set(i,arr.get(i-1));
            arr.set(0,temp);

            }
            else  if(c.equals("C"))
            {

                  temp = arr.get(0);
                  for(i = 0; i < n-1; i++)
                  arr.set(i,arr.get(i + 1));
                  arr.set(n - 1,temp);

            }

        }

        return arr;
    }

 }

Your outer class name should correspond to the type of program.  There's no point in calling it Main since you already have to have a main() method.  Of course, you'll have to be sure that your .java filename and outer class name are the same.
You have a lot of excess whitespace, and I don't think it's a copy/paste error.  Just get rid of all this whitespace so that it's easier for others to read.

On the other hand, this could use more whitespace:

for(int i=0;i<n;i++)

It's good to separate each operator and operand so that the line is easier to read.  Although keeping them together saves horizontal length, the line itself is already short, and even then there's little reason to cram everything together.

for (int i = 0; i < n; i++)

You're using single-character variable names such as n and s, and it's not at all clear what they mean in this code.  If it were a tiny program, then it may be okay.  But for this program, it's best to use meaningful names so that the code is self-documenting.


Notifying if a plane has been found The object is to notify if a plane was found or not.
The aircraft_identifier variable is a must. cpl_id and target_no are optional, only either of them can be applied at time not both, they are used for further identification if aircraft_identifier is not unique.

qo = 'squawk' or 'callsign'  # only these variants are possible
qo_squawk = '12345'
qo_callsign = 'AMAN3'
qo_cplId = '11'
qo_targetNo = '22'

def decision_making(aircraft_identifier, cpl_id=None, target_no=None):
    found_target = False

    if qo == 'callsign' or qo == 'squawk':
        if aircraft_identifier.isdigit():
            found_target = (aircraft_identifier == qo_squawk)
        else:
            found_target = (aircraft_identifier == qo_callsign)

        if found_target:
            if cpl_id:
                found_target = True if qo_cplId == cpl_id else False
            if target_no:
                found_target = True if qo_targetNo == target_no else False

            if found_target:
                print('Found target!')
                return

    print('plane not found in list!')

decision_making('AMAN3', '22')

Can this code be made simpler?

A few suggestions:

“only either can be used at a time not both” – nothing in the function enforces that at most one of these input parameters has been set. Something like this:

if (cpl_id is not None) and (target_no is not None):
     raise ValueError("At most one of cpl_id and target_no should be supplied")

would ensure that at most one of them can be specified by the user.
You have several lines of the form

found_target = foo == bar

I’d put some parentheses around the condition, because I think it makes it easier to read:

found_target = (foo == bar)

Later in the function you have two lines of the form

found_target = True if foo == bar else False

which can be reduced to the construction above.
You have one function that does both business logic (checking whether the plane is correct) and doing some work (printing to screen). You should really separate these two – this function should return True/False, and then the caller should handle printing an appropriate message.
This line:

if qo == 'callsign' or qo == 'squawk':

is a bit unwieldy because you have to add an extra or statement every time you add a new possibility for qo. Better to do something like:

if qo in ['callsign', 'squawk']:

which is easy to extend with more values.

(Although it's not clear why you're doing this check at all, given that qo will always be squawk in the function as defined.)
There are no comments or docstrings. I don’t know anything about planes, so I just have to trust that the conditionals are correct. It would be good to provide some explanation of what this all means – for example, what’s a CPL ID, or what is the qo_ prefix for?


My own quicksort algorithm I will make it more reader friendly after I get the algorithm completed. Does anything with the quick sort algorithm stand out as for why it is not working 100%? This is not homework; I am just practicing problems out of books and I am caught on this one.

Hints, more so than answers, would be more appreciated. Please don't post your own quick sort algorithm, as copying someone else's code does not help me learn. I am just looking for that last bit to help me make the leap across; I don't want you to build another bridge somewhere else, and say "use mine"; it won't help. If my algorithm is a lost cause just say so, and I will go back to the drawing board.

#include <iostream>
#include <vector>
#include <algorithm>
#include <iterator>
#include <cmath>
#include <ctime>
#include <assert.h>

using std::vector;
using std::cin;
using std::cout;
using std::endl;
using std::vector;
using std::ostream_iterator;
using std::istream_iterator;

void print_part( vector<float>::iterator l,
                 vector<float>::iterator r ) {
    copy( l,r+1,ostream_iterator<float>( cout," " ) );
    cout << endl;
}

void print( std::vector<float> &data ) {
    copy( data.begin( ), data.end( ),
          ostream_iterator<float>( cout," " ) );
    cout << endl;
}

void swap( vector<float>::iterator left,
           vector<float>::iterator right ) {
    cout << "Swaping " << (*left) << " with " << (*right) << endl;
    float tmp = (*right);
    (*right) = (*left);
    (*left) = tmp;
}

int get_pivot_index( vector<float> &data,
                     vector<float>::iterator left,
                     vector<float>::iterator right ) {
    int pivot_index = ((right-left)/2);
    int base = std::distance( data.begin( ),left );
    return ( pivot_index+=base );

}

void my_qsort( vector<float> &data,
               vector<float>::iterator left,
               vector<float>::iterator right,
               int side
             ) {
     ((side==0) ? cout<<"":(side==1) ? cout<<"Left: " : cout<<"Right: " );
     print_part( left,right );
     if( (right-left)>0) {
         vector<float>::iterator lit=left;
         vector<float>::iterator rit=right;
         int pivot_index = get_pivot_index( data,left,right );
         int pivot=data[pivot_index];
         cout << "pivot = " << pivot << endl;
         while( lit <= right && rit > left && lit <= rit ) {
             while( lit <= right && (*lit) < pivot ) {
                 if( lit > rit ) { break; }
                 else { lit++; }
             }
             while( rit > left && (*rit) >= pivot ) {
                 if( lit > rit ) { break; }
                 else { rit--; }
            }
            if( lit < rit ) { swap( lit,rit ); }
        }
        my_qsort( data,left,rit,1 );
        my_qsort( data,lit+1,right,2 );
    }
    char c;
    cin >> c;
}

int main( int argc, char *argv[ ] ) {
    vector<float>
    data(
          ( istream_iterator<float>( cin ) ),
          ( istream_iterator<float>( ) )
        );
    cout << "-----------------------------------------------" << endl;
    my_qsort( data,data.begin( ),data.end( )-1,0 );
    cout << "-----------------------------------------------" << endl;
    print( data );
    return ( 0 );
}

The last three libraries should be removed as they're not being used.  There's no need to keep a library around if you're long longer using it or have never used even it.

You may also consider organizing them in some way, such as alphabetically.
You don't really need these:

using std::vector;
using std::cin;
using std::cout;
using std::endl;
using std::vector;
using std::ostream_iterator;
using std::istream_iterator;

Just use std:: where needed, otherwise it'll just get longer if you add more.  Also, you have listed using std::vector twice, but it should just be there once.
print() isn't modifying any data members (it just prints something):

print( std::vector<float> &data )

so data should be passed by const& to avoid an unnecessary copy:

print(std::vector<float> const& data)

There's no need to provide your own swap() function; just use std::swap:

std::swap(lit, rit);

This is not a very common nor maintainable style in my_qsort():

if( lit > rit ) { break; }
else { lit++; }

Put each bracketed statement on its own line, in case you ever need to add to them:

if (lit > rit) {
    break;
}
else {
    lit++;
}

Why is there a "pause" in my_qsort()?

char c;
cin >> c;

Just let the function works without such interruptions.  But if you must keep it anyway, then at least make the user aware that it's a pause, and tell them to input a character in order to continue.
Using parentheses for a simple return is not really needed:

return ( 0 );

Just keep it simple and omit them:

return 0;

Moreover, you don't really need this.  Reaching the end of main() already implies success, so the compiler will insert the return itself.


Binary Converting Code I made a program that converts a decimal number given from the user, and converts it to a binary number. For example, if I entered 2, it would return 10. It then, counts the amount of ones and zeros in the binary conversion. Please tell me any way my code can be either, more efficient, or if there is an error with my code. If there are any bad practices used, please tell me. Indentation is also important to me.

package Conversions;
import java.util.Scanner;

public class BinaryChallenge {
    @SuppressWarnings("resource")
    public static void main(String[] args) {
    int number;

    Scanner scan = new Scanner(System.in);
    int oneCount = 0;
    int zeroCount = 0;
    System.out.println("Enter a positive integer");
    number = scan.nextInt();
    String binary = "";
    if (number < 0) {
        System.out.println("Error: Not a positive integer");
    } else {
        while(number > 0) {
            int mod = (number % 2);
            number /= 2;
            binary = Integer.toString(mod) + binary;
        }
        System.out.println("The binary representation of your integer is: " + binary + ".");
    }

    for (int i = 0; i < binary.length(); i++) {
        if (binary.charAt(i) == '1') {
            oneCount++;
        } else if(binary.charAt(i) == '0') {
            zeroCount++;
        }
    }
    System.out.println("The binary representation of your integer has: " + oneCount + " ones.");
    System.out.println("The binary representation of your integer has: " + zeroCount + " zeros.");
    System.out.println("The binary representation of your integer has 0 twos... duh.");
}
}

Input validation?

At first glance, this looks like some sort of input validation:

if (number < 0) {
    System.out.println("Error: Not a positive integer");
} else {
    // ...
}

// ...
System.out.println("The binary representation of your integer has: " + oneCount + " ones.");
System.out.println("The binary representation of your integer has: " + zeroCount + " zeros.");

But it's not... If the input is negative,
processing will happily continue and print that the number has 0 zeros and 0 ones, which is not true.

Lastly, errors should be printed on System.err instead of System.out.

Variable scope

Declare variables where you really need them, not sooner.
number, zeroCount, oneCount can all be declared later.

Decompose to smaller units

Instead of dumping the code in a single main method,
it would be better to decompose the multiple small methods,
each responsible for one thing.
For example, you could have these methods:

read an int from standard input
convert an int to a binary string
count the number of ones in a binary string

the number of zeros is length minus the number of ones

Simplify

Instead of this:

binary = Integer.toString(mod) + binary;

You could simplify as:

binary = mod + binary;

Reinventing the wheel

Integer.toBinaryString(...) does the same thing as your main loop.


Edit Distance Between Two Strings #include <cassert>
#include <string>
#include <vector>
using namespace std;

inline size_t min(size_t x, size_t y, size_t z)
{
    if (x < y)
        return x < z ? x : z;
    else
        return y < z ? y : z;
}

size_t edit_distance(const string& A, const string& B)
{
    size_t NA = A.size();
    size_t NB = B.size();

    vector<vector<size_t>> M(NA + 1, vector<size_t>(NB + 1));

    for (size_t a = 0; a <= NA; ++a)
        M[a][0] = a;

    for (size_t b = 0; b <= NB; ++b)
        M[0][b] = b;

    for (size_t a = 1; a <= NA; ++a)
        for (size_t b = 1; b <= NB; ++b)
        {
            size_t x = M[a-1][b] + 1;
            size_t y = M[a][b-1] + 1;
            size_t z = M[a-1][b-1] + (A[a-1] == B[b-1] ? 0 : 2);
            M[a][b] = min(x,y,z);
        }

    return M[A.size()][B.size()];
}

int main()
{
    assert(edit_distance("ISLANDER", "SLANDER") == 1);
    assert(edit_distance("MART", "KARMA") == 5);
    assert(edit_distance("KITTEN", "SITTING") == 5);
    assert(edit_distance("INTENTION", "EXECUTION") == 8);
}

Implementation:

It’s generally discouraged to open namespaces (using namespace …;). Instead, import single symbols (using std::vector;) or qualify symbols explicitly.
Three-ways min can be written entirely in terms of std::min:

template <typename T>
T min(T const& a, T const& b, T const& c) {
   return std::min(std::min(a, b), c);
}

“Bug”: the edit distance generally defines the cost for a substitution as 1 (since Levenshtein defined three equivalent edit operations), not 2 which you used in your code;
Algorithmic: Your code needs O(n * m) space. There’s a not too hard to implement variant which requires just O(min{n, m}) due to the fact that for the computation we only need to save the previous column (or row). In fact, with a bit of trickery you can make do with a single vector and two additional variables to save all the active values.

But apart from that it’s a very clean and efficient implementation.


Checking empty object Is there a better method for covering errors in my case? I am looking for best practice for now and future instances.

Foreseen errors that could arise:

No attributes at all
Some attributes could be missing
Attribute value could be empty

// converting dom into array to use later
$fields = $dom->getElementsByTagName($field);
$arr['field'] = array();

foreach ($fields as $field) {
    $attributes = isset($field->attributes) ? $field->attributes : NULL;

    if (!empty($attributes)) {
        // the method getNamedItem($string) returns NULL if not found
        $name = !empty($attributes->getNamedItem('name')) ? $attributes->getNamedItem('name')->nodeValue : NULL;
        $id = !empty($attributes->getNamedItem('id')) ? $attributes->getNamedItem('id')->nodeValue : NULL;
        $field_name = !empty($attributes->getNamedItem('field_name')) ? $attributes->getNamedItem('field_name')->nodeValue : NULL;

        if (!empty($name) && !empty($id) && !empty($field_name)) {
            $arr['field'][$name][$id][$field_name] = $field->nodeValue;
        }
    }
}

I think it is cleaner to handle special cases early; your code winds up less indented.

// converting dom into array to use later
$fields = $dom->getElementsByTagName($field);
$arr['field'] = array();

foreach ($fields as $field) {
  $attributes = isset($field->attributes) ? $field->attributes : NULL;

  if (empty($attributes)) {
    continue;
  }

  $name = !empty($attributes->getNamedItem('name')) ? $attributes->getNamedItem('name')->nodeValue : NULL;
  $id = !empty($attributes->getNamedItem('id')) ? $attributes->getNamedItem('id')->nodeValue : NULL;
  $field_name = !empty($attributes->getNamedItem('field_name')) ? $attributes->getNamedItem('field_name')->nodeValue : NULL;

  if (empty($name) || empty($id) || empty($field_name)) {
    continue;
  }

  $arr['field'][$name][$id][$field_name] = $field->nodeValue;
}

This looks less complicated, at least to me, but it does exactly the same work.

You could also introduce a method to get the nodeValue or NULL, given $attributes and a name.

// converting dom into array to use later
$fields = $dom->getElementsByTagName($field);
$arr['field'] = array();

foreach ($fields as $field) {
  $attributes = isset($field->attributes) ? $field->attributes : NULL;

  if (empty($attributes)) {
    continue;
  }

  $name       = nodeIfPresent($attributes, 'name');
  $id         = nodeIfPresent($attributes, 'id');
  $field_name = nodeIfPresent($attributes, 'field_name');

  if (empty($name) || empty($id) || empty($field_name)) {
    continue;
  }

  $arr['field'][$name][$id][$field_name] = $field->nodeValue;
}


Solving the problem of using directives in a header file with a macro. Is this stupid? I am writing some library code that is mostly templates and so is all contained in header files. I know that placing a using declaration in a header will pollute all the files that include it, but I'm also starting to be really annoyed by having to spell out all the namespaces..
After a bit of tinkering I came up with these macros:

#define MACRO_CONCAT_IMPL( x, y ) x##y
#define MACRO_CONCAT( x, y ) MACRO_CONCAT_IMPL( x, y )

#define PRIVATE_NAMESPACE_IMPL( name ) namespace name { namespace exports{} } using namespace name::exports; namespace name {

#define PRIVATE_NAMESPACE PRIVATE_NAMESPACE_IMPL(MACRO_CONCAT(private_namespace_,__COUNTER__))
#define END_PRIVATE_NAMESPACE }

#define PUBLIC_SECTION namespace exports{
#define END_PUBLIC_SECTION }

To be used like this:

namespace MyNamespace{

PRIVATE_NAMESPACE

    using namespace std;
    using namespace ThirdParty::Library;
    using namespace MyCompany::OtherProduct;

PUBLIC_SECTION

    class Class{
        wstring GetString();
    };
END_PUBLIC_SECTION

END_PRIVATE_NAMESPACE

}

This will create a private namespace with the using declarations and a nested one with the publicly available code that is then imported into the topmost namespace (MyNamespace).

This is different from a plain nested namespace as it expands into this:

namespace MyNamespace{
    namespace private_namespace_0{

      using namespace std;
      using namespace ThirdParty::Library;
      using namespace MyCompany::OtherProduct;

      namespace exports{
            class Class{
                wstring GetString();
            };
        }
    }
    using namespace private_namespace_0::exports;
}

This way the "public" section has access to the "private" one and only the public one is accessible with MyNamespace::.
The weird thing with the counter is to ensure each header has its own private namespace so that declarations are not shared between separate header files. Giving an ID to the namespaces is also the only reason that i'm using macros.

Using a "private" namespace seems to be accepted practice as boost does it too. I also know that __COUNTER__ isn't really portable but vc++ and gcc both support it..

Is there anything really wrong with this idea or a better/standard way to do it?

You're writing a lot of code, but what is the benefit?  How is this any worse?

namespace MyNamespace {
    namespace ImplDetails {
        // Non-public code.
    }
    // Public code.
}

Yes, people can access things in ImplDetails, but you can't get around that sanely.  Obscuring namespace names is only going to hurt compatibility (when a namespace gets renamed after you insert a new one, which will happen eventually).  If people want to access your internals, they will.


Sorting numeric  tags without omitting the attributes I used the following Javascript to sort an unnumbered list of numeric li-tags as follows:

// 1. get the
-element from the BODY
var nList = document.getElementById("allItems");
// 2. extract all the
-elements from that
 and put it in a NodeList
var nEntry = nList.getElementsByTagName('li');

// 3. we can't sort a NodeList, so first make it an Array
var nEntryArray = Array.prototype.slice.call(nEntry, 0);

// 4. sort the array, the normal sort()-function won't do because it is an alphabetical sort
// to sort() numeric values, see http://www.w3schools.com/jsref/jsref_sort.asp example, as "Default sort order is alphabetic and ascending. When numbers are sorted alphabetically, "40" comes before "5".  To perform a numeric sort, you must pass a function as an argument when calling the sort method."
// the numeric value of the
 nodes can be located in nEntryArray[i].firstChild.nodeValue , so compare those

nEntryArray.sort(function(a,b){
  return a.firstChild.nodeValue - b.firstChild.nodeValue
})

// 5. empty the nList and refill it with those in the correct order at the nEntryArray
while (nList.lastChild)
{
  nList.removeChild(nList.lastChild);
}

for (i=0; i<nEntryArray.length; i++)
{
  nList.appendChild(nEntryArray[i]);
}

If a HTML-code is given with a list as such:

100

10

1

20

16

it will order the list numerically without omitting the attributes of the
-tags.

It is one of my first JavaScript-attempts, and I was wondering if this is indeed correct or whether it could be simplified. Especially the conversion from Nodelist to Array seems redundant to me, but I haven't found a more gracious solution.

I post this code-block as I was looking on StackOverflow and through Google for a piece of code to help me solve this particular problem, but I couldn't find something that was understandable to a beginner like me. I easily found ways to sort lists , but then the attributes would be omitted and in the end it took me two days (yep, beginner :-)) to come up with this solution.

Any comments?

There are a few clear improvements I can see when you are reinserting the nodes back into the DOM tree.

for (i=0; i<nEntryArray.length; i++)
{
  nList.appendChild(nEntryArray[i]);
}

1)  You may benefit from using a documentFragment to build up your elements then submit them all at once to the nList

2)  (micro), cache the length of your array in the for loop.

var df = document.createDocumentFragment();
for( var i = 0, l = nEntryArray.length; i < l; i++) {
    df.appendChild(nEntryArray[i]);
}
nList.appendChild(df);

Otherwise I do not see any issues with this code. http://jsfiddle.net/rlemon/faY3Z/


Get start and end dates and strings I have a method to get the time parameter, is this code neat, can the return parameter use other data structures?

private Map<String, Object> getTimeCondition(long smallDaysToSubtract, long bigDaysToSubtract) {

    DateTimeFormatter format = DateTimeFormatter.ofPattern(DateUtil.yyyy_MM_dd);
    ZoneId zoneId = ZoneId.systemDefault();

    LocalDate startLocalDate = LocalDate.now().minusDays(smallDaysToSubtract);
    LocalDate endLocalDate = LocalDate.now().minusDays(bigDaysToSubtract);

    LocalDateTime startLocalDateTime = LocalDateTime.of(startLocalDate, LocalTime.MIN);
    LocalDateTime endLocalDateTime = LocalDateTime.of(endLocalDate, LocalTime.MAX);

    Instant startInstant = startLocalDateTime.atZone(zoneId).toInstant();
    Instant endInstant = endLocalDateTime.atZone(zoneId).toInstant();

    Date startTime = Date.from(startInstant);
    Date endTime = Date.from(endInstant);

    String startDateStr = startLocalDate.format(format);
    String endDateStr = endLocalDate.format(format);

    Map<String, Object> map = new HashMap<>();
    map.put("startTime", startTime);
    map.put("endTime", endTime);
    map.put("startDateStr", startDateStr);
    map.put("endDateStr", endDateStr);

    return map;
}

Hmm... I cannot really make much of this code - what is the pupose of that parameter map?

However, there are a few things to note:

You make use of the modern datetime-api, which is great! And you use it in a correct way - I cannot see any flaws there. BUT in the end you throw all that precision, zoning and niceness overboard and convert everything into old.ugly.Date - why? As long as you use this internally, you should aim for the more precise and powerful api and only convert if necessary for legacy code.
Mashup of different tasks: in most cases, the formatting of data (or dates in this case) belongs to the presentation of an application, whereas the precise values belong to the business logic. Thus, I'd leave the formatting out of the method and move this into the presentation layer, which would also help with the next point...:
Map<String, Object> as a result value. With Object values you throw away all generic goodness a map has, and have to cast the values when you use them. This could be circumvented by eiher heeding my advice in the point above (returning Map<String, LocalDateTime>) or by creating a result class which just consists of the four fields with the correct data types.
Magic strings: "startTime", "endTime", etc. should probably be moved to publicly accessible constants, so the the consumer of the map can use these constants to look up the values he needs without "knowing" and duplicating the concrete key-strings.


SQL Statement with a Select Statement Inside a Where Clause Is there a more efficient way to write the following PLSQL statement? It seems inefficient to me to have the SQL statement nested inside the where clause.

SELECT REQ_ID
FROM REQUESTS R
WHERE P_REQ_ID IS NULL AND
      EXISTS (
        SELECT *
        FROM REQUESTS
        WHERE P_REQ_ID = R.REQ_ID
        )

Here is what I ended up with:

Select Distinct rParent.REQ_ID
From REQUESTS rParent
Inner Join REQUESTS rChild On rChild.P_REQ_ID = rParent.REQ_ID
Where rParent.P_REQ_ID IS NULL


OO design for Tic Tac Toe program I am practicing object oriented design and have taken Tic Tac Toe as an example. I have written first all the requirements and then started writing code. I would like to get it reviewed so that I can improve my skill further. Please provide suggestions.

I have placed code at this GitHub repo.

Square.js

//Square class
function Square(){
    this.symbol = "";
    this.isOccupied = false;
}

//Based on player turn set symbols
Square.prototype.setSymbol = function(value){
    if(value !== 'X' && value !== 'O'){
        return 'Please enter X or O';
    }else if(this.isOccupied){
        return 'This square is already filler'
    }else{
        this.symbol = value;
        this.isOccupied = true;
    }
};

Square.prototype.getSymbol = function(){
    return this.symbol;
};

Board.js

//Board Class
function Board(sqaures){
    this.squares = sqaures;
    this.grid = [];
    this.filledSquare = 0;
}

//Builds board based on number of squares
Board.prototype.buildBoard = function(){
    var i,j;
    for(i = 0; i < this.squares; i++){
        this.grid[i] = [];
        for(j = 0; j < this.squares; j++){
            this.grid[i][j] = new Square();
        }
    }
};

//Updates board on each players turn
Board.prototype.update = function(row, column, symbol){
    this.grid[row][column].setSymbol(symbol);
    this.filledSquare++;
};

//Show the current state of board
Board.prototype.displayBoard = function(){
    var i, j,brd = "";
    for(i = 0; i < this.squares; i++){
        for(j = 0; j < this.squares; j++){
            brd += this.grid[i][j].getSymbol() +' - ';
        }
        brd += '\n';
    }
    console.log(brd);
};

Player.js

//Player Class
function Player(name,symbol){
    this.name = name;
    this.symbol = symbol;
}

tttgame.js

//The game class which governs the play
function TTTGame(squares){
    this.board = new Board(squares);
    this.board.buildBoard();
    this.players = [];
    this.players[0] = new Player('John','X');
    this.players[1] = new Player('Doe', 'O');

    this.getCurrentPlayer = function(){
        return (this.board.filledSquare %2 === 0 ? 'X' : 'O');
    };

    //I have written this as dummy method which should be replaced with actual logic
    this.play = function(){
        this.board.displayBoard();
        var currentPlayer = this.getCurrentPlayer();
        var row, col, symbol;
        this.board.update(0,1,currentPlayer);
        currentPlayer = this.getCurrentPlayer();
        this.board.update(0,0,currentPlayer);
        currentPlayer = this.getCurrentPlayer();
        this.board.update(1,1,currentPlayer);
        currentPlayer = this.getCurrentPlayer();
        this.board.update(2,1,currentPlayer);
        currentPlayer = this.getCurrentPlayer();
        this.board.update(1,0,currentPlayer);
        this.board.displayBoard();
    }

    //It will contain method to decide winner or tie
}

GameStarter.js

//Starts the game
(function GameStarter(){
    var tttGame = new TTTGame(3);
    tttGame.play();
})();

Overall this looks pretty good. Just a couple of things come to mind:

Square.js

The setSymbol method should throw errors instead of returning strings, especially since I don't see any place where those strings are handled in a manor allowing the user to correct their mistake.
The error message when the square is occupied just needs a slight grammatical correction: This square is already filled (filled instead of filler)

TTTGame.js

Create all methods on the prototype. You aren't using any private function scope variables, so putting the methods on the prototype will be more efficient.

General Comments

The name TTTGame is kind of obtuse. I know it is probably an abbreviation for "Tick Tack Toe Game" but I think a more descriptive name is in order. All around, throwing these classes into a namespace would be useful. Consider the semantic differences between these two sets of classes:

Square (is it a tick tack toe square or a two dimensional representation of a plane in mathematics)
Board (Diving board? Game board? Discussion board?)
TTTGame (What does TTT stand for?)

Vs:

TickTackToe.Square
TickTackToe.Board
TickTackToe.Game

No explanation necessary for the second set of class names.

All in all, this looks pretty good.


Implementing a SQL like structure in Python, changing values I am implementing a SQL-like datbase system in python, and so I needed to implement a value changing function, but what I ended up with seems very convoluted.

Here is code relevant to the value changing:

def _get_primary_rowval(self):
    idx = 0
    for n in self.def_column:
        if n.is_primary:
            return idx
        idx += 1

def _get_row_val_by_ident(self, ident):
    idx = 0
    idents = []
    for n in self.def_column:
        if n.identifier == ident:
            return idx
        idents.append(n.identifier)
        idx += 1
    raise AttributeError("No Columns have the identifier given.  Identifier given: {0}, Identifiers Possible: {1}".format(ident, idents))

def change_val(self, col_ident, new_val, primary_value):
    for n in self.rows:
        if n.contents[self._get_primary_rowval()] == primary_value:
            n.contents[self._get_row_val_by_ident(col_ident)] = new_val

Here is the code for the Row and Column Classes:

class TableObject(object):

    def __init__(self, ident):
        self.identifier = ident

class Column(TableObject):

    def __init__(self, ident, arg_type, primary = False):
        super().__init__(ident)
        self.arg_type = arg_type
        self.is_primary = primary

    def check_type(self, applicant_object):
        if type(applicant_object) == self.arg_type:
            return True
        else:
            return False

class Row(TableObject):

    def __init__(self, contents):
        self.contents = []
        for n in contents:
            self.contents.append(n)

    def get_contents(self):
        return self.contents

I was wondering if there was a better/more Pythonic way to do this.

Column index lookups

If I understand your intentions correctly,
the _get_primary_rowval and _get_row_val_by_ident functions are used to find column indexes.
They iterate over the list of column definitions until they find a match.
Do the columns get reordered often? Or ever?
In a table-like structure, I would expect the columns to not change much,
and so the column indexes should not change either.
Rather than looping to get the indexes,
it would be better to store them in a dictionary,
so that lookups become \$O(1)\$ operations.

Consider for example this use of these methods:

def change_val(self, col_ident, new_val, primary_value):
    for n in self.rows:
        if n.contents[self._get_primary_rowval()] == primary_value:
            n.contents[self._get_row_val_by_ident(col_ident)] = new_val

For every row,
those _get methods are called,
every time looping over the list of columns.
This is very inefficient and an unusual thing to do.

I suggest to rework this logic:
use a dictionary to store the column indexes.
If the columns change, update the dictionary accordingly.

Use boolean values directly

Instead of this:

def check_type(self, applicant_object):
    if type(applicant_object) == self.arg_type:
        return True
    else:
        return False

You can do simply this:

def check_type(self, applicant_object):
    return type(applicant_object) == self.arg_type

Copying a list

Instead of this:

def __init__(self, contents):
    self.contents = []
    for n in contents:
        self.contents.append(n)

Use the .copy method:

self.contents = contents.copy()


Code for a Person DTO, with tests My needs are to manage DTO in a php5.3 application. I've written few lines of code in TDD with phpunit 6. The scenario I want to implement is a request with following fields:

name
surname
password
alias
email
address
civic_number
country

And I need to create DTO's with

credentials
info
address

Provide DTO properties

Each DTO should provide its own properties.

public function testProvideDtoPropertyNames()
{
    $propertyNames = Person::getPropertyNames();

    $expectedProperties = array(
        'name',
        'surname',
    );

    $this->assertEquals(
        $expectedProperties,
        $propertyNames
    );
}

Getters

I also want that all properties are available via getters:

public function testProvidePropertyViaGeneralGetter()
{
    $dto = Person::createFromArray(array(
        'name' => 'Simone',
    ));

    $this->assertEquals(
        'Simone',
        $dto->get('name')
    );
}

Filtering

The main purpose here is to filter all fields and just keep consideration of the DTO's properties:

public function testDTOAcceptOnlyItsOwnProperties()
{
    $dto = Person::createFromArray(array(
        'name' => 'Simone',
        'sfadfsa' => 'Simone',
    ));

    $expectedProperties = array(
        'name' => 'Simone',
        'surname' => null,
    );

    $this->assertEquals(
        $expectedProperties,
        $dto->asArray()
    );
}

Serialization

Serializing and un serializing I want that my code works fine and without side effect.

public function testSerializationKeepSameProperties()
{
    $dto = Person::createFromArray(array(
        'name' => 'Simone',
        'surname' => 'Gentili',
        'sadfasdrname' => 'Gentili',
    ));

    $serialized = serialize($dto);
    $unserialized = unserialize($serialized);

    $this->assertEquals(
        $dto->asArray(),
        $unserialized->asArray()
    );

    $this->assertEquals(
        array(
            'name' => 'Simone',
            'surname' => 'Gentili',
        ),
        $unserialized->asArray()
    );
}

Are those tests good? I hope yes but any suggestions are welcome.

Finally, ... the code:

A DTO can be easily done with a simple class with public properties:

class Person extends Dto
{
    public $name;
    public $surname;
}

Here the production code:

abstract class Dto
{
    private $properties;

    private function __construct(array $properties)
    {
        $this->properties = $properties;
    }

    public static function createFromArray($properties)
    {
        return new static($properties);
    }

    public function set($propertyName, $propertyValue)
    {
        $this->$propertyName = $propertyValue;
    }

    public function get($propertyName)
    {
        return isset($this->properties[$propertyName])
            ? $this->properties[$propertyName]
            : null;
    }

    public static function getPropertyNames()
    {
        $reflected = new ReflectionClass(new static(array()));

        $properties = $reflected->getProperties(
            ReflectionProperty::IS_PUBLIC
        );

        return array_map(function ($item) {
            return $item->getName();
        }, $properties);
    }

    public function asArray()
    {
        $properties = array();

        foreach (static::getPropertyNames() as $itemValue) {
            $properties[$itemValue] = $this->get($itemValue);
        }

        return $properties;
    }

    public function __sleep()
    {
        foreach (self::getPropertyNames() as $propertyName) {
            $this->set(
                $propertyName,
                $this->get($propertyName)
            );
        }

        return self::getPropertyNames();
    }

    public function __wakeup()
    {
        foreach (self::getPropertyNames() as $propertyName) {
            $this->properties[$propertyName] = $this->$propertyName;
            $this->$propertyName = null;
        }

        return self::getPropertyNames();
    }
}

In the tests (and anywhere else), you should use constants to ensure correct spelling of the properties :

const PERSON_NAME = 'name';
const PERSON_SURNAME = 'surname';
...
$expectedProperties = array(self::PERSON_NAME,self::PERSON_SURNAME);

Then the Filtering and Serialization tests are very similar while they should be atomic (one test tests one thing) : you should not test Filtering whike testing Serialization.

In the production code there is one error : you set the desired property (i.e. $this->name) while you get the internal array's key ($this->properties['name']). To keep the simplicity of defining an object as a class with public properties you should drop the internal array. This will also make the __sleep and __wakeup calls much clearer.

Also a design flaw is to use the Reflection : it seems unavoidable but depending on your usage there might be performance issues. Using a caching system such as a static array will help. In particular if you look at the __wakeup and __sleep function you will notice a repeted call to the getPropertyNames method.

Finally what might be problematic is that you don't check if the property exists when setting or getting. If it does not you should throw an exception. There are magic methods in PHP5.3 (__get, __set and __call) ; you may do better using them. This way you will be able to use $personName = $person->getName() / $person->setName('Simone') as well as $personName = $person->name / $person->name = 'Simone'

Tests :

const PERSON_NAME = 'name';
const PERSON_SURNAME = 'surname';

public function testProvideDtoPropertyNames()
{
    $propertyNames = Person::getPropertyNames();

    $expectedProperties = array(
        self::PERSON_NAME,
        self::PERSON_SURNAME,
    );

    $this->assertEquals(
        $expectedProperties,
        $propertyNames
    );
}
public function testProvidePropertyViaGeneralGetter()
{
    $dto = Person::createFromArray(array(
        self::PERSON_NAME => 'Simone',
    ));

    $this->assertEquals(
        'Simone',
        $dto->getName()
    );
}
public function testDTOAcceptOnlyItsOwnProperties()
{
    $dto = Person::createFromArray(array(
        self::PERSON_NAME => 'Simone',
        'non existent property' => 'Simone',
    ));

    $expectedProperties = array(
        self::PERSON_NAME => 'Simone',
        self::PERSON_SURNAME => null,
    );

    $this->assertEquals(
        $expectedProperties,
        $dto->asArray()
    );
}
public function testSerializationKeepSameProperties()
{
    $properties = array(
        self::PERSON_NAME => 'Simone',
        self::PERSON_SURNAME => null,
    );

    $dto = Person::createFromArray($properties);

    $serialized = serialize($dto);
    $unserialized = unserialize($serialized);

    $this->assertEquals(
        $dto->asArray(),
        $unserialized->asArray()
    );

    $this->assertEquals(
        $properties,
        $unserialized->asArray()
    );
}

Production code :

class DtoException extends Exception
{
}
abstract class Dto
{
    private static $propertyNamesByClass = array();

    private function __construct(array $properties)
    {
        foreach($properties as $propertyName=>$propertyValue)
            if(Static::hasPropertyName($propertyName))
                $this->set($propertyName,$propertyValue);
    }
    public function __call($method, array $arguments)
    {
        $getOrSet = substr($method, 0, 3);
        if($getOrSet != 'get' && $getOrSet != 'set')
            throw new DtoException('"'.get_class($this).'" has no method "'.$method.'"');

        $propertyName = strtolower(substr($method, 3));
        if(!Static::hasPropertyName($propertyName))
            throw new DtoException('"'.get_class($this).'" has no property "'.$propertyName.'"');

        $getOrSetMethod = array($this,$getOrSet);
        array_unshift($arguments,$propertyName);
        return call_user_func_array($getOrSetMethod, $arguments);
    }
    public function __get($propertyName)
    {
        if(!Static::hasPropertyName($propertyName))
            throw new DtoException('"'.get_class($this).'" has no property "'.$propertyName.'"');

        return $this->get($propertyName);
    }
    public function __set($propertyName, $propertyValue)
    {
        if(!Static::hasPropertyName($propertyName))
            throw new DtoException('"'.get_class($this).'" has no property "'.$propertyName.'"');

        return $this->set($propertyName, $propertyValue);
    }

    public static function createFromArray($properties)
    {
        return new Static($properties);
    }

    public function set($propertyName, $propertyValue)
    {
        $this->$propertyName = $propertyValue;
    }

    public function get($propertyName)
    {
        return $this->$propertyName;
    }

    public static function getPropertyNames()
    {
        $className = get_called_class();
        if(isset(self::$propertyNamesByClass[$className]))
            return self::$propertyNamesByClass[$className];

        $reflected = new ReflectionClass($className);
        $reflectedProperties = $reflected->getProperties(
            ReflectionProperty::IS_PUBLIC
        );

        $propertyNames = array();
        foreach($reflectedProperties as $reflectedProperty) {
            $propertyNames[] = $reflectedProperty->getName();
        }

        self::$propertyNamesByClass[$className] = $propertyNames;
        return $propertyNames;
    }
    public static function hasPropertyName($propertyName)
    {
        $propertyNames = Static::getPropertyNames();
        return in_array($propertyName,$propertyNames);
    }

    public function asArray()
    {
        $values = array();

        foreach (Static::getPropertyNames() as $propertyName) {
            $values[$propertyName] = $this->get($propertyName);
        }

        return $values;
    }

    public function __sleep()
    {
        $propertyNames = self::getPropertyNames();

        foreach ($propertyNames as $propertyName) {
            $propertyValue = $this->get($propertyName);
            $this->set($propertyName, $propertyValue);
        }

        return $propertyNames;
    }

    public function __wakeup()
    {
        $propertyNames = self::getPropertyNames();

        return $propertyNames;
    }
}

I tested it on PHP5.3 and it works, however I have to tell you that it does not work on PHP5.2- or PHP7+.


any better way than using old servlet api for downloads I am wondering if the following code can be modified to be used with latest version of spring APIs?
The following code works fine. However, I wanted to know if servlet APIs can be avoided which i have used in the code below.
public void download (HttpServletRequest request, HttpServletResponse response, @PathVariable String filename, @PathVariable String user) throws IOException{

            String param1 = filename;
            String param2 = user;

            //The following URL will need to be updated with "prod" or "dev" or "test" depending upon the environment
            final String FILE_LOCATION = "/mnt/nfs/myfolder/prod/documents/custom_documents/"+param2;

            if(param1 == null || param2 == null) {
                // The Request Parameters Were Not Present In The Query String. Do Something Or Exception Handling !!
                System.out.println("Request Parameter Not Found in first if!");
            } else if ("".equals(param1) || "".equals(param2)) {
                // The Request Parameters Were Present In The Query String But Has No Value. Do Something Or  Exception Handling !!
                System.out.println("Request Parameter Not Found in second if!");
            } else {

                String fileName = (String) param1;
                String contentType = getContentType(fileName.split("\\.")[1]);
                File file = new File(FILE_LOCATION + "/" +fileName);
                response.setContentType(contentType);
                response.addHeader("Content-Disposition","attachment; filename=" + fileName);
                response.setContentLength((int) file.length());
                ServletOutputStream servletOutputStream = response.getOutputStream();
                BufferedInputStream bufferedInputStream = new BufferedInputStream(new FileInputStream(file));
                int bytesRead = bufferedInputStream.read();
                while (bytesRead != -1) {
                    servletOutputStream.write(bytesRead);
                    bytesRead = bufferedInputStream.read();
                }
                if (servletOutputStream != null) servletOutputStream.close();
                if(bufferedInputStream != null) bufferedInputStream.close();

            }

    }

private String getContentType(String fileType) {
        String returnType = null;
        for(int i = 0; i < contentTypes.length; i++) {
            if(fileType.equals(contentTypes[i][0])) returnType = contentTypes[i][1];
        }
        return returnType;
    }

Resource interface provides an effective way of accessing the content of the resource and supports handling of all kinds of file resources. Please see if that helps you to some extent.
public ResponseEntity < Resource > downloadFile(String fileName) {

  final String FILE_LOCATION = "/mnt/nfs/myfolder/prod/documents/custom_documents/";

  File file = new File(FILE_LOCATION + "/" + fileName);

  Path path = Paths.get(file.getAbsolutePath());
  ByteArrayResource resource = new ByteArrayResource(Files.readAllBytes(path));

  HttpHeaders headers = new HttpHeaders();
  //headers.add(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=" + file.getName());

  return ResponseEntity.ok().headers(headers).contentLength(file.length()).contentType(MediaType.APPLICATION_OCTET_STREAM).body(resource);
}


HTTP request path parser I have written a method to tokenize HTTP request paths such as /employee/23/edit:

protected void compile(String path){
    int mark=0;
    for(int i=0; i<path.length(); ++i){
        if(path.charAt(i)==DELIM){
            if(mark!=i)
                tokens.add(path.substring(mark,i));
            mark=i+1;
        }
        else if(path.length()==i+1){
            tokens.add(path.substring(mark,i+1));
        }
    }
}

And a method to tokenize the consumer of these paths such as /employee/[id]/edit:

protected void compile(String path){
    int mark=0;
    boolean wild=false;
    for(int i=0; i<path.length(); ++i){
        if(!wild){
            if(path.charAt(i)==DELIM){
                if(mark!=i)
                    tokens.add(path.substring(mark,i));
                mark=i+1;
            }
            else if(path.length()==i+1){
                tokens.add(path.substring(mark,i+1));
            }
            else if(path.charAt(i)=='['){
                wild=true;
            }
        }
        else if(path.charAt(i)==']'){
            tokens.add("?");
            wild=false;
            mark=i+1;
        }
    }
}

The idea here is that there will be an implicit variable called id with the value 23. However, that isn't here nor there. How does my approach look? Can it be improved? Also: DELIM = '/'.

This is more-or-less an exercise in writing a parser, which is why I didn't use String#split().

Your first compile method can be replaced by a single call to String.split.

Assuming the intended behavior for the second compile method is such that "/foo/b[a]r/baz" will compile to {"foo", "?", "baz"}, it can be replaced by a call to split and then iterating over the result and replacing any string the includes square brackets with "?".

If the intended behavior is rather that it will compile to {"foo", "b", "?", "r", "baz"}, you can first replace [anything] by /?/ using String.replace and then use String.split.


PHP Class code that allow developers to register taxonomies in a CMS Building complex applications can sometimes be a pain when extending or editing later on; I read online about the S.O.L.I.D principles of OOD, I want to understand if I am following a good approach that I can follow when developing the rest of the app.

I need answers to the following questions

Is the code well-documented?
The use of namespaces, Was it necessary ?
Does the class violates any of the SOLID principles ?
$ic = new static; Is it wrong to use this approach for inline usage?
Using static methods for easy access in a single line of code to creating and getting taxonomies, Are there better ideas ?
App\Taxonomy::register("categories")->isHierarchical()->produce() The usage, Is it developer friendly to you ? Or are there other approaches developers would rather ?

Some of the ideas I had in mind when writing down the Class:

I used namespaces because the project will have many classes and sub packages that would need categorizing for better usage by developers when they develop mini apps (plugins).
I used static methods for single line usage to make it easier for developers to register new taxonomies and set options into one line of code.

Here is the class

namespace App;

/**
 * Class Taxonomy
 *
 * @package App
 */

class Taxonomy {

    /**
     * Store all the taxonomies
     *
     * @since 0.5
     *
     * @var array
     */
    protected static $taxonomies = array();

    /**
     * Default settings for creating taxonomies
     *
     * @since 0.5
     *
     * @var array
     */
    protected static $tax_defaults = array(
        'key' => null,
        'hierarchical' => false
    );

    /**
     * A Temporary variable that holds the taxonomy
     * settings until the taxonomy is being produced
     * then the temporary variable is being reset
     * to an empty array
     *
     * @since 0.5
     *
     * @var array
     */
    protected static $tax = array();

    /**
     * Temporarily holds an error because
     * in some cases errors occurs when for example
     * using a duplicate taxonomy key
     *
     * @since 0.5
     *
     * @var bool
     */
    protected static $error = false;

    /**
     * Register a new taxonomy to be later on
     * added to a data set
     *
     * @since 0.5
     *
     * @param $taxonomy_key String Unique key used for the taxonomy as an identifier
     * @return static
     */
    public static function register( $taxonomy_key ){
        $ic = new static;
        self::reset();

        if( isset( self::$taxonomies[ $taxonomy_key ] ) )
            self::$error = "taxonomy-already-registered";

        self::$tax = self::$tax_defaults;
        self::$tax['key'] = $taxonomy_key;

        return $ic;
    }

    /**
     * Set the taxonomy to hierarchical
     *
     * @since 0.5
     *
     * @return static
     */
    static function isHierarchical(){
        $ic = new static;
        self::$tax['hierarchical'] = true;
        return $ic;
    }

    /**
     * Final step, This method is used to store
     * the taxonomy to App global taxonomies variable
     *
     * @since 0.5
     *
     * @return bool
     */
    static function produce(){
        if( self::$error !== false )
            return self::$error;

        $key = self::$tax['key'];
        self::$taxonomies[$key] = self::$tax;
        self::reset();
    }

    /**
     * Return the object of the Taxonomy
     *
     * @since 0.5
     *
     * @param $taxonomy_key
     * @return bool
     */
    static function get( $taxonomy_key ){
        return isset( self::$taxonomies[$taxonomy_key] ) ? self::$taxonomies[$taxonomy_key] : false;
    }

    /**
     * Reset temporary variables
     *
     * @since 0.5
     */
    protected static function reset(){
        self::$tax = array();
    }
}

Usage example

// Register Taxonomies
App\Taxonomy::register("categories")->isHierarchical()->produce();
App\Taxonomy::register("tags")->produce();

// Get Taxonomies
App\Taxonomy::get("tags"); // Will return an array of the information registered

I think you're mixing too many ideas here and as a consequence your class ends up having too many responsibilities, which than need extensive documentation because their behaviour may not be obvious and self explanatory.

Try to keep things just as simple as possible. In your case a simple plain old PHP object would potentially do the job.

Take for example the following entity class, which is basically the core of your code, all the rest of your functions are just creational util function.

class Taxonomy
{
    private $name;
    private $isHierarchical;

    public function __construct($name, $isHierarchical = false)
    {
        $this->name           = $name;
        $this->isHierarchical = $isHierarchical;
    }

    public function getName()
    {
        return $this->name;
    }

    public function isHierarchical()
    {
        return $this->isHierarchical;
    }
}

Because the code is that simple and self explanatory I don't even need to add any comment. Generally it's preferable to have clear and readable code over extensive documentation ...

That little extra bit of logic to have unique taxonomy names within an array of taxonomies could be archived by using for example a TaxonomyCollection object which takes care of that.

E.g.

class TaxonomyCollection
{
    private $taxonomies = [];

    public function __construct(array $taxonomies)
    {
        array_walk($taxonomies, function($taxonomy) {
            $this->addTaxonomy($taxonomy);
        });
    }

    public function addTaxonomy(Taxonomy $taxonomy)
    {
        if ($this->getTaxonomyByName($taxonomy->getName())) {
            throw new \RuntimeException(
                sprintf(
                    'Taxonomy name "%s" not unique within the collection.',
                    $taxonomy->getName()
                )
            );
        }

        array_push($this->taxonomies, $taxonomy);
    }

    public function getTaxonomies()
    {
        return $this->taxonomies;
    }

    public function getTaxonomyByName($taxonomyName)
    {
        foreach($this->taxonomies as $taxonomy) {
            if ($taxonomyName === $taxonomy->getName()) {
                return $taxonomy;
            }
        }
        return null;
    }
}

Example usage:

new TaxonomyCollection([
    new Taxonomy('foo', true),
    new Taxonomy('bar'),
]);

Now if you like to have a more convenient way for creating objects have a look on the factory pattern for example, but don't pollute your entities with endless boilerplate.


Writing a clean Predicate Builder I have an inner class for filtering:
private class RegSpecification implements Specification<Reg> {

    private final transient RegFilterDTO filter;
    private transient Predicate predicate;
    private transient CriteriaBuilder cb;

    public RegSpecification(RegFilterDTO filter) {
        this.filter = filter;
    }

    @Override
    public Predicate toPredicate(Root<Reg> root, CriteriaQuery<?> cq, CriteriaBuilder cb) {
        // this.cb = cb;
        this.predicate = cb.conjunction();

        Join<Reg, Acct> userJoin = root.join(acct, JoinType.INNER);

        predicate = eqEnumVal(predicate, cb, root.get(regType), RegType.class, filter.getRegType());
        predicate = eq(predicate, cb, userJoin.get(Acct_.id), filter.getAcctId());
        predicate = like(predicate, cb, root.get(regNumber), filter.getRegNumber());

        return predicate;
    }
}

And I made a helper class, to be used by other class as well who implements filtering by Specification (methods are public since some services are in other packages):
public class PredicateBuilder {

    public static Predicate like(Predicate predicate, CriteriaBuilder cb, Path<String> path, String value) {
        if (StringUtils.isNotEmpty(value)) {
            return cb.and(predicate, cb.like(cb.lower(path), "%" + value.toLowerCase() + "%"));
        }
        return predicate;
    }

    public static <T> Predicate eq(Predicate predicate, CriteriaBuilder cb, Path<T> path, T value) {
        if (value != null) {
            return cb.and(predicate, cb.equal(path, value));
        }
        return predicate;
    }

    public static <T extends Enum<T>> Predicate eqEnumVal(Predicate predicate, CriteriaBuilder cb,
                                                          Path<T> path, Class<T> enumType, String name) {
        if (StringUtils.isNotEmpty(name)) {
            return eq(predicate, cb, path, Enum.valueOf(enumType, name));
        }
        return predicate;
    }

}

Question is
Can I use private local variables in my RegSpecification so I won't have to use predicate = ... every time I call the helper class and some generic method to pass the predicate and cb to the helper class (so I won't have to pass the predicate and cb in my helper class methods)?
Where I could just do something like:
PredicateBuilder.with(predicate, cb)
   ...
   like(userJoin.get(Acct_.id), filter.getAcctId());
   eq(userJoin.get(Acct_.id), filter.getAcctId());
   like(root.get(regNumber), filter.getRegNumber());
   ...
.build();

in my toPredicate method.
Or any suggestions to make the code cleaner?
(Should I also annotate the helper class with @Component?)

You could follow the below approach:
public class PredicateBuilder {

    private Predicate predicate;
    private CriteriaBuilder cb;
    private Runnable[] tasks;

    private static final PredicateBuilder instance = new PredicateBuilder();
    private PredicateBuilder(){
    }

    public static PredicateBuilder with(Predicate p, CriteriaBuilder c) {
        instance.predicate = p;
        instance.cb = c;
        return instance;
    }

    public PredicateBuilder tasks(Runnable... tasks) {
        instance.tasks = tasks;
        return instance;
    }

    public Predicate build() {
        Arrays.stream(instance.tasks)
              .forEach(Runnable::run);
        return instance.predicate;
    }

    public static void like(Path<String> path, String value) {
        if (StringUtils.isNotEmpty(value)) {
            CriteriaBuilder c = instance.cb;
            instance.predicate = c.and(instance.predicate,
                                       c.like(c.lower(path), "%" + value.toLowerCase() + "%"));
        }
    }

    public static <T> void eq(Path<T> path, T value) {
        if (value != null) {
            CriteriaBuilder c = instance.cb;
            instance.predicate = c.and(instance.predicate, c.equal(path, value));
        }
    }

    public static <T extends Enum<T>> void eqEnumVal(Path<T> path, Class<T> enumType, String name) {
        if (StringUtils.isNotEmpty(name)) {
            instance.predicate = eq(instance.predicate, instance.cb, path, Enum.valueOf(enumType, name));
        }
    }

}

Then you could invoke as below:
return PredicateBuilder.with(predicate, cb)
                       .tasks(() -> eqEnumVal(root.get(regType), RegType.class, filter.getRegType()),
                              () -> eq(userJoin.get(Acct_.id), filter.getAcctId()),
                              () -> like(root.get(regNumber), filter.getRegNumber())
                       .build();

```


Handling a maze in JavaScript Is there a way to shorten the conditional statement below? As you can see, there's a lot of repetition. Is the if-statement the best approach here? Bearing in mind that there will likely be more numbers to check for (not, just 2 and 3):

  var searchArea = function() {
    // Search the area around the current position for hidden doors
    if(detectWall('left') == 2) {
        status.innerHTML = "Hidden Door to the left";
    } else if (detectWall('right') == 2) {
        status.innerHTML = "Hidden door to the right";
    } else if (detectWall('up') == 2) {
        status.innerHTML = "Hidden door above you";
    } else if (detectWall('down') == 2) {
        status.innerHTML = "Hidden door below you";
    } else if (detectWall('right') == 3 || detectWall('left') == 3 || detectWall('up') == 3 || detectWall('down') == 3) {
        status.innerHTML = "You are close to a fountain";
    }

}

var detectWall = function(dir) {
    // Detect walls from the array
    switch(dir) {
        case 'right':
            return mapArray[parseInt(player.y/20)][parseInt((player.x+20)/20)]
        case 'left':
            return mapArray[parseInt(player.y/20)][parseInt((player.x-20)/20)]
        case 'up':
            return mapArray[parseInt((player.y-20)/20)][parseInt(player.x/20)]
        case 'down':
            return mapArray[parseInt((player.y+20)/20)][parseInt(player.x/20)]
        default:
            return false
    }

}

Whenever you're dealing with a tile map like this, you should make your best effort to avoid writing code for each direction, replacing it with generalized operations.

The most basic such generalization is to use vectors like [-1, 0] instead of code specific to a direction.

You can create tables of all direction-specific things, like this:

var left  = {vector: [-1, 0], name: "left",  reference: "to the left"};
var right = {vector: [ 1, 0], name: "right", reference: "to the right"};
var up    = {vector: [0, -1], name: "up",    reference: "above you"};
var down  = {vector: [0,  1], name: "down",  reference: "below you"};
var directions = [left, right, up, down];

Now your search function can avoid all repetition:

var WALL = 2;
var FOUNTAIN = 3;

function searchArea() {
  // Search the area around the current position for hidden doors
  var foundThing = "";
  directions.some(function (direction) {
    if (detectWall(direction) === WALL) {
      foundThing = "Hidden Door " + direction.reference;
      return true;  // functions like 'break' in a .some(...)
    }
  });
  if (!foundThing) {
    directions.some(function (direction) {
      if (detectWall(direction) === FOUNTAIN) {
        foundThing = "You are close to a fountain";
        return true;
      }
    });
  }
  status.textContent = foundThing;
}

I moved the tile types into constants WALL and FOUNTAIN so that the code is more readable.

I used some to loop over the array, so instead of break as in a for loop, I used return true (some loops until the provided function returns true, and we don't use the return value of some) and an if statement to skip over the second search. This could be cleaned up some; in particular if you had many things to search for it would be good to have a generalization of "search for many things and stop on the first one" just like we are now generalizing over directions. But this is good enough for this simple case.

Note also that I replaced innerHTML with textContent. Never use innerHTML unless there is no possible substitute for it; if you don't know exactly what you're doing it's a great way to get security bugs.

Here's the revised detectWall:

var tileSize = 20;
function detectWall(direction) {
  var x = Math.floor(player.x / tileSize) + direction.vector[0];
  var y = Math.floor(player.y / tileSize) + direction.vector[1];
  return mapArray[y][x];
}

I have removed the parseInt calls and replaced them with a more direct way to get the result, Math.floor. Note that the vector elements have magnitude 1 instead of a tile size; this allows it to be reused in more ways, such as in your player motion control.

I have pulled out the tile size value into a constant. This is a good thing to do everywhere, so that if you decide you want different tile sizes you only have to change it in one place.

A common additional thing to do is to check whether the computed tile position is out of the bounds of mapArray and if so return a specific value. For example, return the WALL value, so that all of the edges of the map act like walls. I haven't done this here because I don't know how your bounds are defined.


Set same color as prev row if data matches otherwise different color I have a requirement in which I need to set color of two or more rows of a table same if they have same value in cell one and I have to maintain this alternatively i.e. no 3 or 4 rows should have the same color unless there cell value is same.

I have written following code but I don't like it that much as it seems dirty.

function SetGroupingDetails(row, item, color, namedColor) {
  $(row)
    .css("background-color", color) //set different color for different groups
    .addClass(namedColor);
}

$("#tblExport tbody tr").each(function() {
  var item = $("td:first", this).text();

  var prev = $(this).prev();

  if (prev.length > 0) {
    var prevItem = prev.find('td:first').text();

    if (prevItem == item) {
      if (prev.hasClass('Green'))
        SetGroupingDetails(this, item, "#009933", "Green");
      else
        SetGroupingDetails(this, item, "#cc9900", "Orange");
    } else {
      if (prev.hasClass('Orange'))
        SetGroupingDetails(this, item, "#009933", "Green");
      else
        SetGroupingDetails(this, item, "#cc9900", "Orange");
    }
  } else
    SetGroupingDetails(this, item, "#009933", "Green");
});
#tblExport body {
  font: normal medium/1.4 sans-serif;
}
#tblExport {
  border-collapse: collapse;
  width: 100%;
}
#tblExport th,
td {
  padding: 0.25rem;
  text-align: left;
  border: 1px solid #ccc;
}
#tblExport th {
  background: #bfbfbf;
}

        Item No.

        Item Name

        10178601

        X

        10178601

        X

          40062595

          Y

          40062595

          Y

          80214549

          Z

          80214549

          Z

Any suggestions?

As previously mentioned, #tblExport body should probably be #tblExport > tbody, and you should trim() text before comparing.

It is possible to reduce the five calls to SetGroupingDetails().  If you don't try to set a background-color manually, then it's a simple matter of adding either a "zebra-even" class or a "zebra-odd" class.  Simply toggle between them whenever the text doesn't match its predecessor.

var c = 0;
$("#tblExport tbody tr").each(function() {
    var $item = $("td:first", this);
    var $prev = $(this).prev().find('td:first');

    if ($prev.length && $prev.text().trim() != $item.text().trim()) {
        c = 1 - c;
    }
    $(this).addClass(['zebra-even', 'zebra-odd'][c]);
});
#tblExport > tbody {
  font: normal medium/1.4 sans-serif;
}
#tblExport {
  border-collapse: collapse;
  width: 100%;
}
#tblExport th,
td {
  padding: 0.25rem;
  text-align: left;
  border: 1px solid #ccc;
}
#tblExport th {
  background: #bfbfbf;
}
#tblExport > tbody > tr.zebra-even {
  background-color: #093;
}
#tblExport > tbody > tr.zebra-odd {
  background-color: #c90;
}

        Item No.

        Item Name

        10178601

        X

        10178601

        X

          40062595

          Y

          40062595

          Y

          80214549

          Z

          80214549

          Z


Entering users into a MySQLi database This is a form that I want to use to enter users into a database. I used MySQLi with prepared statements. I want to make sure that I sanitized my data well.

<?php
/*
 * To change this license header, choose License Headers in Project Properties.
 * To change this template file, choose Tools | Templates
 * and open the template in the editor.
 */
require_once './db_con.php';
$id = '';
$firstname = trim(filter_input(INPUT_POST, 'firstname', FILTER_SANITIZE_FULL_SPECIAL_CHARS));
$lastname = trim(filter_input(INPUT_POST, 'lastname', FILTER_SANITIZE_FULL_SPECIAL_CHARS));
$email = filter_input(INPUT_POST, 'email', FILTER_SANITIZE_EMAIL);
//$val_email = trim(filter_var($email, FILTER_VALIDATE_EMAIL));
$password = trim(filter_input(INPUT_POST, 'password', FILTER_SANITIZE_FULL_SPECIAL_CHARS));
$password2 = trim(filter_input(INPUT_POST, 'password2', FILTER_SANITIZE_FULL_SPECIAL_CHARS));
$errs = array();
#firstname
    if ( empty($firstname) ) {
        $errs[] = "Please enter your first name.";
    }

#lastname
    if ( empty($lastname) ) {
        $errs[] = "Please enter your last name.";
    }

#email
    if ( empty($email) ) {
        $errs[] = "Please enter your E-mail.";
    }
#email validation
    if ( !filter_var($email, FILTER_VALIDATE_EMAIL) ) {
        $errs[] = "Please enter a valid E-mail.";
    }
#check if email exist
    if ( email_exists($email,$connection) ) {
        $errs[] = "Your E-mail already exists.";
    }

#password
    if ( empty($password) ) {
        $errs[] = "Please enter your password.";
    } elseif (strlen($password) < 6) {
        $errs[] = "Your password must have  at least 6 characters.";
}
#confirm-password
    if ( empty($password2) ) {
        $errs[] = "Please confirm your password.";
    } else {
#matching-passwords
        if ( $password !== $password2 ) {
            $errs[] = "Password didn't match.";
        }
    }

if ( !empty( $errs ) )  {
    foreach ($errs as $err) {
        echo $err.'</br >';
    }
} else {

    $hashed_password = password_hash($password, PASSWORD_DEFAULT);

    $stmt = $connection->prepare("INSERT INTO `users` (`id`, `firstname`, `lastname`, `email`, `password`) VALUES (?, ?, ?, ?, ?)");
    $stmt->bind_param("sssss", $id, $firstname, $lastname, $email, $hashed_password);
    if ( !($stmt->execute()) ) {
        echo "There was a problem, Try again.";
    } else {
        echo "You are registered now.";
    }

    $stmt->close();
}
#functions
function email_exists($email,$connection) {

    $stmt = $connection->prepare("SELECT * FROM `users` WHERE `email` = ?");
    $stmt->bind_param("s", $email);
    $stmt->execute();
    $result = $stmt->get_result();
    $rows_count = $result->num_rows;

    if ( $rows_count >= 1 ) {
        return TRUE;
    }

    $stmt->close();
}

This code is perfectly safe. The only issue with sanitization in this code is that it makes the data, so to say, overly-sanitized.

The way you are getting your input variables, makes them suitable for the HTML output. But there is no HTML output in the code present. Which makes such santitization slightly misplaced. It is considered a better practice to "sanitize"  the data right before use, instead of doing that prematurely.

The way you are doing it now could result in some undesired consequences.    For example, imagine a honest girl named Sarah O'Hara going to register with your site. Your code will make her name "Sarah O&#039;Hara" to be saved in the database. Next time she will try search for her name, a search for "Sarah O'Hara" will find nothing.

Therefore it is recommended to sanitize a variable just before the actual use. E.g. making it suitable for the HTML output right before the actual output. Given modern template engines such as Twig are doing an auto-escaping, it will take you no trouble having the user data properly sanitized.

In this regard I would get input variables without any HTML related sanitization, I.e.

$firstname = trim(filter_input(INPUT_POST, 'firstname'));
$lastname = trim(filter_input(INPUT_POST, 'lastname'));

whereas actual sanitization being done before the actual output.


Suffix array construction in \$ O(n \log^2 n) \$ Here is my implementation of the suffix array construction algorithm which follows this paper (specifically, pages 4 and 6).

#!/usr/bin/env python3

from math import ceil, log2

def sufar(txt):

    txt = txt + chr(0)
    N, tokens = len(txt), sorted(set(t for t in txt))

    equivalence = {t: i for i, t in enumerate(tokens)}

    c, r = [equivalence[t] for t in txt], [(0, 0, 0)]

    for i in range(1, ceil(log2(N)) + 1):
        n = 2 ** (i - 1)

        r = sorted([(c[j],  c[(j + n) % N], j) for j in range(N)])

        c[r[0][2]] = 0
        for j in range(1, N):
            c[r[j][2]] = c[r[j - 1][2]]
            if r[j][0:2] != r[j - 1][0:2]:
                c[r[j][2]] += 1

    return [result[2] for result in r][1:]

This implementation looks correct, here are the unit tests I have tried.

My concerns are:

txt = txt + chr(0) to add a special symbol $ which is not supposed to be in txt.
using if r[j][0:2] != r[j - 1][0:2] to understand if the suffixes at positions j and j - 1 are from the same equivalence class. I would expect the comparison to be if r[j][0:1] != r[j - 1][0:1] but that fails the unit tests.

1. Review

I'm reviewing the updated version of the code only.

There's no docstring. What does this function do? How do I call it? What does it return? Can you give an example of what it does?
The names are obscure. We're not running out of letters, so suffix_array would be clearer to the reader than sufar. (You said in comments that this was to make it "quicker to type" but in that case maybe you need a better editor? Most programming editors these days have some kind of completion facility on names.)
The algorithm is tricky enough that it needs explanation. My advice would be to add comments to the loop explaining the invariants that holds at each iteration. (Figuring out loop invariants is invaluable for understand how an algorithm works, and they can be turned into assertions that check the correctness of the implementation.)
The code does not actually implement the algorithm in the Vladu/Negruşeri paper. A comparison with their C implementation on page 6 shows that there are two points of difference.

(i) When preparing the L array (result in sufar), if i + cnt is out of bounds (j + n in sufar), the dummy element is used, but in sufar the index is wrapped around to get cls[(j + n) % N].

(ii) When computing the new P array (cls in sufar), if L[i] differs from L[i - 1] then the entry in P is assigned the value i. However, in sufar, the entry in cls is assigned the value cls[result[j - 1][2]] + 1.

Nonetheless, despite these differences in the implementation, sufar gets the correct result!

In case (i) that's because the original algorithm imagines extending the string with enough dummy characters to make the suffixes the same length (thus abac becomes abac$$$$ and the 4-character suffixes are abac, bac$, ac$$, c$$$, and $$$$ which gets discarded), whereas sufar images extending the string with a single dummy character and then a repeat of itself (thus abac becomes abac$aba and the 4-character suffixes are  abac, bac$, ac$a, c$ab and $aba which gets discarded). But the single $ is enough for these to come out in the correct order.

In case (ii) we only care about the sorted order of the entries in P, not their actual values, so the change makes no difference.

I think that difference (ii) is fine, but (i) is sufficiently tricky that I would avoid it if at all possible. (Or else write a comment explaining why I did it.)
To make it easy for the reader to check that you've correctly implemented the algorithm in the paper, I would use the same notation as the paper unless there was a very good reason not to. So txt → A, cls → P, result → L, N → n, n → count, etc.
Instead of set(t for t in txt), just write set(txt).
Instead of adding chr(0) to txt and then making equivalence classes for the entries in txt (which relies on chr(0) not already appearing in txt), make the equivalence class first, and then append an equivalence class that you know is not present, for example -1.
Instead of computing ceil(log2(N)), start with count = 1 and double it each iteration until it is too large. This avoids the dependence on the math module.
It turns out that there's no need to actually add a dummy value to the text in the first place. If we avoid this, then we can also avoid adding 1 to N and avoid having to drop the initial element of result at the end.
The initialization of the cls array based on the sorted equivalence classes of the length-1 substrings of txt is really just a special case of the way that cls is updated in the loop. If the loop were turned upside down so that it computes cls first and then result, you could arrange to avoid the special initialization of cls. See below for how this is done.
Another benefit of turning the loop upside down is that it avoids a wasteful computation of cls on the last iteration that will never be used.
Numeric tuple lookups like the [2] in result[j][2] are hard to understand. It's usually better to unpack the tuple into local variables so that you can refer to its elements by name.
In Python it's usually best to avoid iterating over sequence indices, if possible, and to prefer iterating over the elements directly. Here the computation of cls involves iterating over pairs of adjacent elements in result; you could use the pairwise recipe from the itertools module, or combine zip and itertools.islice as I've done below.

2. Revised code

from itertools import chain, islice

def suffix_array(A):
    """Return a list of the starting positions of the suffixes of the
    sequence A in sorted order.

    For example, the suffixes of ABAC, in sorted order, are ABAC, AC,
    BAC and C, starting at positions 0, 2, 1, and 3 respectively:

    >>> suffix_array('ABAC')
    [0, 2, 1, 3]

    """
    # This implements the algorithm of Vladu and Negruşeri; see
    # http://web.stanford.edu/class/cs97si/suffix-array.pdf

    L = sorted((a, i) for i, a in enumerate(A))
    n = len(A)
    count = 1
    while count < n:
        # Invariant: L is now a list of pairs such that L[i][1] is the
        # starting position in A of the i'th substring of length
        # 'count' in sorted order. (Where we imagine A to be extended
        # with dummy elements as necessary.)

        P = [0] * n
        for (r, i), (s, j) in zip(L, islice(L, 1, None)):
            P[j] = P[i] + (r != s)

        # Invariant: P[i] is now the position of A[i:i+count] in the
        # sorted list of unique substrings of A of length 'count'.

        L = sorted(chain((((P[i],  P[i+count]), i) for i in range(n - count)),
                         (((P[i], -1), i) for i in range(n - count, n))))
        count *= 2
    return [i for _, i in L]


Using a service interface to get user input from dialogs I haven't worked with WPF/MVVM for a while, but now I'm back fixing some issues with one of my old apps and remembering the struggles I always had when dealing with asking for user input (eg. showing a dialog) from a view model.

I've put some thought into it and decided to try routing user input requests through a service interface so that it can be mocked/tested, while keeping the view logic out of my view model.

public interface IUserInputService
{
    UserRegistration PromptUserRegistration();
}

I then wrote a generic method that creates a new instance of a view and view model, connects them together, shows the dialog and then returns the dialog view model and result (eg. accepted, cancelled) back to the caller. Finally, the caller can extract information from the returned view model and return the actual desired result.
The view models have events which are called when the view model is "done", eg: Accepted and Cancelled. These are hooked up using the generic method.

public class UserInputService : IUserInputService
{
    // Implementation of the service interface using the generic method below
    public UserRegistration PromptUserRegistration()
    {
        var result = ShowDialog<UserRegistrationDialog, UserRegistrationViewModel>((v, vm) =>
        {
            vm.Accepted += (s, e) => v.DialogResult = true;
            vm.Cancelled += (s, e) => v.DialogResult = false;
        });

        if (result.State != DialogState.Accepted)
        {
            return null;
        }

        return new UserRegistration
        {
            FirstName = result.ViewModel.FirstName,
            LastName = result.ViewModel.LastName
        };
    }

    // The generic method
    private DialogResult<TViewModel> ShowDialog<TView, TViewModel>(
        Action<TView, TViewModel> viewModelConnected
    )
        where TView : Window, new()
        where TViewModel : ViewModelBase, new()
    {
        TViewModel viewModel = new TViewModel();
        TView view = new TView();
        view.DataContext = viewModel;

        viewModelConnected(view, viewModel);

        view.ShowDialog();

        DialogState state;
        switch (view.DialogResult)
        {
            case true:
                state = DialogState.Accepted;
                break;
            case false:
                state = DialogState.Declined;
                break;
            default:
                state = DialogState.Cancelled;
                break;
        }

        return new DialogResult<TViewModel>(state, viewModel);
    }
}

Now I can pass around the service interface using DI/IoC and view models can ask for user input without caring how (eg. via dialog or mocking).

The other types (DialogResult and DialogState) are these:

public enum DialogState
{
    Accepted,
    Declined,
    Cancelled
}

public class DialogResult<TViewModel>
{
    public DialogResult(DialogState state, TViewModel viewModel)
    {
        State = state;
        ViewModel = viewModel;
    }

    public DialogState State { get; }
    public TViewModel ViewModel { get; }
}

So my questions are...

Is this a good approach?
What would you do differently and why?
Any other suggestions regarding code quality?

I want to stick to the principles of MVVM as much as possible (though within reason) while keeping things testable... Let me know what you think.

I have worked in a project where such an IUserInputService was used. After few years, the implementation was a big class containing the dialog calling logic for all modules.

Today, I would prefer a generic service which is able to connect a view with the view model without implementing the logic for each concrete dialog.

Code Review

Your code looks clean and tidy
The communication between view model and the view via events may be OK in your case, because the view model is just an internal structure not known outside the method. However, in generell I would prefer a way where the view is not references by the view model (wich is indirect the case because the delegates contain a reference to the view). Usually, I use a view model property "bool Close" and bind it to the view so that the view can react on changes and close itself.
Your approach does not allow creation of view models via DI container. If the VM has dependencies, they must be passed to the method PromptUserRegistration.
Configuring the VM with default values requires to parameterize the PromptUserRegistration as well.

All things considered, I would prefer an approach where the view model is created / configured by the calling logic and shown via an generic service because it is more flexible and less distributed code.

Note that there are also frameworks available that adress that problem. E.g. https://github.com/FantasticFiasco/mvvm-dialogs


Quiz game with timer This is my first Go program. I would like to know what could be improved, what is done wrong, and anything else that I should know. The CSV contains 10 questions with the 10 answers separated by a comma. For example:

10+10,20
4+5,32

If you want the full context this is from gophercises which are some exercises to practice Go.

package main

import (
    "encoding/csv"
    "flag"
    "fmt"
    "math/rand"
    "os"
    "strings"
    "time"
)

var points int

func main() {
    fPtr := flag.String("csv", "problems.csv", "FileName in csv (question, answer)")
    tPtr := flag.Int("time", 10, "Time in seconds")
    sPtr := flag.Bool("shuffle", true, "shuffle your questions order")
    flag.Parse()
    questions := readCSV(*fPtr)

    fmt.Print("Press enter to start you have ", *tPtr, " seconds")
    fmt.Scanln()

    askQuestions(&questions, *tPtr, *sPtr)

    fmt.Println("You got ", points, " points")
}

func askQuestions(questions *[][]string, time int, s bool) {

    go startTimer(time)
    for _, i := range shuffle(s, len(*questions)) {
        fmt.Println("Question ", (*questions)[i][0], ":")
        var a string
        fmt.Scan(&a)
        a = strings.ToLower(strings.Trim(a, " "))
        if a == (*questions)[i][1] {
            points++
        }
    }
}

func shuffle(shuffle bool, sliceLen int) []int {
    var r []int
    for i := 0; i < sliceLen; i++ {
        r = append(r, i)
    }
    if shuffle {
        rand.Seed(time.Now().Unix())
        rand.Shuffle(len(r), func(i, j int) {
            r[i], r[j] = r[j], r[i]
        })
    }

    return r
}

func readCSV(s string) (questions [][]string) {
    f, err1 := os.Open(s)
    records, err2 := csv.NewReader(f).ReadAll()
    for err1 != nil || err2 != nil {
        fmt.Println("Error: ", err1, err2)
        fmt.Println("Please re-enter the name of the CSV file: ")
        fmt.Scan(&s)
        f, err1 = os.Open(s)
        records, err2 = csv.NewReader(f).ReadAll()
    }
    return records
}

func startTimer(seconds int) {
    time.Sleep(time.Duration(seconds) * time.Second)
    fmt.Println("Time is up! You got ", points, " points")
    os.Exit(0)
}

startTimer()

The time package provides timers natively. So rather than using a call to time.Sleep(), you can use time.NewTimer().

Use fmt.Printf() rather than fmt.Println() for printing formatted strings.

func startTimer(seconds int) {
    time.Sleep(time.Duration(seconds) * time.Second)
    fmt.Println("Time is up! You got ", points, " points")
    os.Exit(0)
}

Becomes:

func startTimer(sec int) {
    timer := time.NewTimer(time.Duration(sec) * time.Second)
    <-timer.C
    fmt.Printf("Time is up! You got %d points\n", points)
    os.Exit(0)
}

readCSV()

In readCSV(), you have multiple error variables. You can instead use a single error variable as you go along.

I advise against adding file retry logic. It complicates the code, and most users can simply re-run the command upon error. Instead, you should return an error value.

Using a named return (questions) means you should actually use the questions variable. Because I switch to multiple return values, I removed the named parameter.

Also be sure to close the file after you're done reading from it.

func readCSV(s string) (questions [][]string) {
    f, err1 := os.Open(s)
    records, err2 := csv.NewReader(f).ReadAll()
    for err1 != nil || err2 != nil {
        fmt.Println("Error: ", err1, err2)
        fmt.Println("Please re-enter the name of the CSV file: ")
        fmt.Scan(&s)
        f, err1 = os.Open(s)
        records, err2 = csv.NewReader(f).ReadAll()
    }
    return records
}

Becomes:

func readCSV(name string) ([][]string, error) {
    f, err := os.Open(name)

    if err != nil {
        return nil, err
    }

    qs, err := csv.NewReader(f).ReadAll()

    if err != nil {
        return nil, err
    }

    return qs, f.Close()
}

askQuestions()

Again, here you should use fmt.Printf() to print formatted strings.  You also use the value i as an index, but with the range keyword you can access the currently indexed value, see here.

You should check the return of fmt.Scan().

Rather than starting the timer within askQuestions(), I opted to move it to main(). This means we can remove the time argument.

main()

Naming variables like fPtr and sPtr is just extra typing. Documenting the type in the name itself is not very useful.

fPtr becomes name
tPtr becomes time
sPtr becomes shuffle

From your updated code with shuffling:

Rather than keeping a second copy of the shuffled questions, just modify the array in-place. You also don't need to seed rand (see this example from the Go docs).

Conclusion

Here is the code I ended up with:

package main

import (
    "encoding/csv"
    "flag"
    "fmt"
    "log"
    "math/rand"
    "os"
    "time"
)

var points int

func main() {
    name := flag.String("csv", "problems.csv",
        "filename in csv (question, answer)")
    time := flag.Int("time", 10, "time in seconds")
    shuffle := flag.Bool("shuffle", true, "shuffle your question order")

    flag.Parse()

    questions, err := readCSV(*name)

    if err != nil {
        log.Fatal(err)
    }

    if *shuffle {
        rand.Shuffle(len(questions), func(i, j int) {
            questions[i], questions[j] = questions[j], questions[i]
        })
    }

    go startTimer(*time)

    if err := askQuestions(&questions); err != nil {
        log.Fatal(err)
    }

    fmt.Printf("You got %d points\n", points)
}

func readCSV(name string) ([][]string, error) {
    f, err := os.Open(name)

    if err != nil {
        return nil, err
    }

    qs, err := csv.NewReader(f).ReadAll()

    if err != nil {
        return nil, err
    }

    return qs, f.Close()
}

func askQuestions(questions *[][]string) error {
    for i, q := range *questions {
        fmt.Printf("Question %d: %s\n", i+1, q[0])

        var a string
        _, err := fmt.Scan(&a)

        if err != nil {
            return err
        }

        if a == q[1] {
            points++
        }
    }

    return nil
}

func startTimer(sec int) {
    timer := time.NewTimer(time.Duration(sec) * time.Second)
    <-timer.C
    fmt.Printf("\nTime is up! You got %d points\n", points)
    os.Exit(0)
}

Hope this helps!


Sum array values based on conditions in other arrays I have the following code that sums the values in wgt_dif (a numpy array) if certain conditions in two other numpy arrays are met. It is basically the equivalent of a SUMIFS function in Excel.

sum_4s = 0

for i in range(len(pntl)):
    if pntl[i] == 4 and adj_wgt[i] != max_wgt:
        sum_4s += wgt_dif[i]

I'm wondering if there is a more Pythonic way to write this. It works fine, but I'm new to Python and numpy and would like to expand my "vocabulary".

sum([w for w, p, a in zip(wgt_dif, pntl, adj_wgt) if p == 4 and a != max_wgt])

Explanation:

zip(a, b, c)

creates the list of triplets of corresponding values from the lists a, b, c - something as

[(a[0], b[0], c[0]), (a[1], b[1], c[1]), (a[2], b[2], c[2]), ...]

so the part

for w, p, a in zip(wgt_dif, pntl, adj_wgt)

loops over this triples, associating th 1st item to w, 2nd to p, and 3rd to a.