Finding the largest value in a 2D array I'm pretty new to Java and programming in general and I'm doing practice problems from my textbook over the summer to try to get ahead. This is a simple program that finds the largest value in a 2D array. I'm just looking for feedback on the organization of my code as well as any improvements I can make so that I can build solid habits going forward.

import java.util.Scanner;

public class Main {

    public static void main(String[] args) {

    Scanner input = new Scanner(System.in);
    System.out.println("Enter the number of rows and columns in the array: ");

    //Creates a temporary two-dimensional array whose size is the next two inputed integers
    double[][] temp = new double[input.nextInt()][input.nextInt()];

    //Populates the temp array with inputed values
    System.out.println("Enter the array: ");
    for(int i=0; i < temp.length; i++) {
        for(int j=0; j < temp[0].length; j++) {
            temp[i][j] = input.nextDouble();
        }
    }
    input.close();

    //Creates a new Location object that is equal to the object returned from the locateLargest() method call
    Location location = Location.locateLargest(temp);

    System.out.println("The location of the largest element is " + location.maxValue + " at (" + location.row + ", " + location.column + ")");

    }
}

public class Location {
    public int row=0, column=0;
    public double maxValue = 0;

    public Location() {
    }

    /*
     * Accepts a two-dimensional array as the parameter.
     * Creates a new Location object.
     * Returns a Location object with maxValue at position (0,0) unless a larger value is found.
     */
        public static Location locateLargest(double[][] a) {
        Location location = new Location();
        location.maxValue=a[0][0];

            for(int i = 0; i < a.length; i++) {
                for(int j=0; j < a[i].length; j++) {
                    if(a[i][j] > location.maxValue) {
                        location.maxValue = a[i][j];
                        location.row = i;
                        location.column = j;
                    }
                }

            }

            return location;
       }

    }

Obligatory disclaimer: My opinion.

I can see that you are a beginner - it's a little hard to explain what you should work on, because the path you're taking toward finding the highest value in an array is a little hard to understand.

Normally, you'd just need to do something like

public static double findHighestValue(double[][] doubles) {
    double currentHighestValue = Double.MIN_VALUE;
    for (int row = 0; row < doubles.length; row++) {
        for (int col = 0; col < doubles[row].length; col++) {
            double value = doubles[row][col];
            if (value > currentHighestValue) {
                currentHighestValue = value;
            }
        }
    }
    return currentHighestValue;
}

to find the maximum value of a 2D array (which is the question), but instead, you're returning an object Location.

I can understand where you got the class name idea -- locating a value -- however, it's counter-intuitive. Instead, a class name such as Case or Square would be a bit more descriptive due to the fact that the noun location has multiple meanings.

I'll show you what I would do to answer the question (finds the largest value in a 2D array), and then, to help you learn, I'll give you some tips on the code you already wrote.

As this question is extremely wide, I won't go as far as to explain everything, but I'll give you a few pointers.

What I would do

This isn't a really complex problem per se, so there's not a lot to explain.

Since you want to know the coordinates of the maximum value (column and row), we still need to create a custom class.

First, you must determine what the class represents.
In this case, it represents an element in a multidimensional array, so like I said earlier, I'll just use something a little bit more intuitive like a Case.

Why is Case a better name?

If you visualize the 2D array, it looks like this:

 _ _ _
|_|_|_|
|_|_|_|
|_|_|_|

And each square looks like a case.

Each case contains a value, and a position determined by the row number and the column number of the case, so those 3 parameters will be our fields:

public class Case {

    private int row;
    private int col;
    private double value;

    public Case(int row, int col, double value) {
        this.row = row;
        this.col = col;
        this.value = value;
    }

    public int getRow() {
        return row;
    }

    public void setRow(int row) {
        this.row = row;
    }

    public int getCol() {
        return col;
    }

    public void setCol(int col) {
        this.col = col;
    }

    public double getValue() {
        return value;
    }

    public void setValue(double value) {
        this.value = value;
    }

}

With this new class in mind, the method we would use to find the case with the highest value would be something like this:

public static Case findHighestValue(double[][] doubles) {
    Case highestCase = new Case(-1, -1, Double.MIN_VALUE);
    for (int row = 0; row < doubles.length; row++) {
        for (int col = 0; col < doubles[row].length; col++) {
            double value = doubles[row][col];
            if (value > highestCase.getValue()) {
                highestCase = new Case(row, col, value);
            }
        }
    }
    return highestCase;
}

I would put that in an interface named something like CaseUtils (unless you have another class related to cases, e.g. CaseService, but since you initially only had a Main class and a Location class, I assume there isn't any), which would look like this:

public interface CaseUtils {

    static Case findHighestValue(double[][] doubles) {
        Case highestCase = new Case(-1, -1, Double.MIN_VALUE);
        for (int row = 0; row < doubles.length; row++) {
            for (int col = 0; col < doubles[row].length; col++) {
                double value = doubles[row][col];
                if (value > highestCase.getValue()) {
                    highestCase = new Case(row, col, value);
                }
            }
        }
        return highestCase;
    }

}

That way, if you want to add more methods of the same type (e.g. findLowestValue), you'll be able to add them there too.

As for how to implement it in your main, you just need to do something like this:

Case c = CaseUtils.findHighestValue(array);
System.out.println("The position of the case with the highest value is " + c.getValue()
        + " at (" + c.getRow() + ", " + c.getCol() + ")");

Tips on your current code

Your comments on the method locateLargest do not conform with the javadoc convention:

/*
 * Accepts a two-dimensional array as the parameter.
 * Creates a new Location object.
 * Returns a Location object with maxValue at position (0,0) unless a larger value is found.
 */

should instead be something like:

/**
 * Locates the largest double
 * @param a two-dimensional array as the parameter.
 * @return Location object with maxValue at position (0,0) unless a larger value is found.
 */

If you want to read more about Javadoc, I suggest you read this article.

The class Location should not have public fields, but private fields with public getters and setters instead. As for why, you can search online, but this is also a good explanation: Why do we need private variables?

If you have any questions, feel free to ask.

NOTE

I used the name Case to highlight that the class name should clearly explain what it is. However, I wouldn't personally use that class name and instead, I would use Cell because the keyword case is reserved (switch case).


Increment the neighbors of a cell in a matrix Let's say you want to increment the cells around the position (x,y) in a square matrix m of size T.

I've written the same code in a few languages. Here's the JS syntax version :

if (x>0) {
    if (y>0) m[x-1][y-1]++;
    m[x-1][y]++;
    if (y<T-1) m[x-1][y+1]++;
}
if (y>0) m[x][y-1]++;
if (y<T-1) m[x][y+1]++;
if (x<T-1) {
    if (y>0) m[x+1][y-1]++;
    m[x+1][y]++;
    if (y<T-1) m[x+1][y+1]++;
}

I don't like this kind of code, those tests look redundant. What bothers me is less the clarity of the code than the fact it has to do so many times the same checks.

But I don't see anything better. Am I just missing an obvious solution or is there nothing better with the branching toolset we have ? A slower code isn't acceptable as an answer, performance is the essence of the question.

Like Simon I would store the directions into a data structure, though I would probably hard code it

//Array Index Constants
var X = 0 , Y = 1;
//All 8 neighbours ( vectors )
var vectors = [[-1,-1],[-1,0],[-1,1],[0,-1],[0,1],[1,-1],[1,0],[1,1]];

The fun thing about JavaScript is that there is no ArrayIndexOutOfBoundsException in JavaScript, requesting a value out of bounds returns undefined.

So, now, you can go for

for( int i = 0 ; i < vectors.length ; i++ ){
  var vector = vectors[i];
  if( m[x+vector[X]] && m[x+vector[X]][y+vector[Y]] !== undefined )
    m[x+vector[X]][y+vector[Y]]++;
}

I did use for m[x+vector[X]] a falsey comparison, if the row X was defined it would pass the falsey evaluation ( it would be an Array ). For m[x+vector[X]][y+vector[Y]] I compared to undefined, since 0 is a realistic, acceptable value which would evaluate as false.

If I had to do a lot of matrix magic, then I would have a dedicated function to check whether a point is part of the matrix. And a dedicated function to add a vector to a point.


Optimizing search by id in some data in JSON structure I have an object created by a JSON data. I need to retrieve the id for an object, I would like to know if a faster way exist, considering I am using for ..in loop.

var test =  {
     id: 0,
      cnt: [
        {
          id: 1,
          cnt: [
            {
              id: 2,
              cnt: [
                {
                  id: 3,
                  cnt:[]
                }
              ]
            }
          ]
        },

      ]
    };

    getById: function (id) {
                function getByIdInner(obj, id) {
                    var result;
                    for (var p in obj) {
                        if (obj.id == id) {
                            return obj;
                        } else {
                            if (typeof obj[p] === 'object') {
                                result = getByIdInner(obj[p], id);
                                if (result) {
                                    return result;
                                }
                            }
                        }
                    }
                    return result;
                }
                return getByIdInner(test, id);

   },

It looks like this is just a snippet of the code you are using, but if this is the standard structure, you could simplify your search by doing something like:

getById: function(id) {
    function innerGetById(obj, id) {
        var result;
        if (obj.id == id) {
            return obj;
        } else {
            for (var i = 0; i < obj.cnt.length; i++) {
                result = innerGetById(obj.cnt[i], id);
                if (result) {
                   return result;
                }
            }
        }
    }
    return innerGetById(test, id);
}

This will only work if you know the names of the keys you are dealing with, if that is unknown, you will need to do a for in loop like you have. The key here is that you are testing for the id outside of the loop through the array.

I would also define innerGetById outside of your getById function if possible. I just wrote it this way because I don't know the context that you are defining the getById function in and it looks like you may be creating it as part of an object.

To answer your question, I'm not sure that a faster way exists than manually searching through the structure unless you are willing to index the structure before doing the search. If you want to do that, you can use the same sort of recursive method, but create a flat object with the ids as keys and the associated object as the value. This will make lookups very simple.

You can turn your test object into something like this:

lookupTable = {
    0: { id: 0, cnt: [ ... ] },
    1: { id: 1, cnt: [ ... ] },
    2: { id: 2, cnt: [ ... ] },
    etc.
}

By using something like:

var lookupTable = {};
function buildLookupTable(obj) {
    lookupTable[obj.id] = obj;
    for (var i = 0; i < obj.cnt.length; i++) {
         buildLookupTable(obj.cnt[i]);
    }
}
buildLookupTable(test); // Using your original test data

Then your lookup is simply: lookupTable[id]


Extracting time duration in the session from 30 million rows I am looking for making my code faster. I am working on yoochoose recsys 2015 dataset.. and trying to perform some transformations.. [recsys2015], it has got 30 million plus rows of data. The goal of my code is to extract time duration in the session with the help of python, and the day , hour segment etc, however I realized that my code with pandas is taking hell lot of time, any alternative approaches is well appreciated
below is sample data and code
Data:
sessioid,timestamp,itemid,category
1,2014-04-07T10:51:09.277Z,214536502,0
1,2014-04-07T10:54:09.868Z,214536500,0
1,2014-04-07T10:54:46.998Z,214536506,0
1,2014-04-07T10:57:00.306Z,214577561,0
2,2014-04-07T13:56:37.614Z,214662742,0
2,2014-04-07T13:57:19.373Z,214662742,0
2,2014-04-07T13:58:37.446Z,214825110,0
2,2014-04-07T13:59:50.710Z,214757390,0

Code
import pandas as pd
import datetime as dt
clickspath='/tmp/gensim/yoochoose/yoochoose-clicks.dat'
buyspath='/tmp/gensim/yoochoose/yoochoose-buys.dat'
clicksdat=pd.read_csv(clickspath,header=None)
clicksdat.columns=['Sid','Timestamp','itemid','itemcategory']
buysdat=pd.read_csv(buyspath,header=None)
buysdat.columns=['Sid','Timestamp','itemid','price','qty']
segment={}
for i in range(24):
    if i<7:
        segment[i]='EM'
    elif i<10:
        segment[i]='M'
    elif i<13:
        segment[i]='A'
    elif i<18:
        segment[i]='E'
    elif i<23:
        segment[i]='N'
    elif i<25:
        segment[i]='MN'
#*******************************************
buyersession=buysdat.Sid.unique()
clickersession=clicksdat.Sid.unique()
with open('/tmp/gensim/yoochooseclick_new.dat','w') as f1:
    for session in clickersession:
        purchased=1 if session in buyersession else 0
        temp=clicksdat[clicksdat.Sid==session]
        maxtime=dt.datetime.strptime(temp.Timestamp.max(),"%Y-%m-%dT%H:%M:%S.%fZ")
        mintime=dt.datetime.strptime(temp.Timestamp.min(),"%Y-%m-%dT%H:%M:%S.%fZ")
        day=maxtime.day
        month=maxtime.month
        noofnavigations=len(temp.Timestamp)-1
        duration=int((maxtime-mintime).total_seconds())
        totalcats=len(temp.itemcategory.unique())
        totalitems=len(temp.itemid.unique())
        timesegment= segment[maxtime.hour]
        segmentchange=1 if (segment[maxtime.hour]!=segment[mintime.hour]) else 0
        f1.write(str(purchased)+','+str(day)+','+str(month)+','+str(noofnavigations)+','+str(duration)+','+str(totalcats)+','+str(totalitems)+','+str(timesegment)+','+str(segmentchange)+'\n')

Note: This works on both Python 2 and Python 3

A few basic points:
The bool datatype is basically just an int and therefore 1 == True and 0 == False. This makes your two ternaries easier to read, because instead of
purchased=1 if session in buyersession else 0
segmentchange=1 if (segment[maxtime.hour]!=segment[mintime.hour]) else 0

You can just write
purchased = session in buyersession
segmentchange = segment[maxtime.hour] != segment[mintime.hour]

noofnavigations is a bad name. You should at least call it no_of_navigations, to go along with PEP8. But why not just navigations?
While we're here, you might as well comply with PEP8 on the other things as well (mainly, put spaces around all infix operators and equal signs).

If you demote all of your temporary variables (which you only use once anyways) to keys in a dictionary you can make the printing a lot easier:
with open('/tmp/gensim/yoochooseclick_new.dat', 'w') as f1:
    for session in clickersession:
        temp = clicksdat[clicksdat.Sid == session]
        maxtime = dt.datetime.strptime(temp.Timestamp.max(), "%Y-%m-%dT%H:%M:%S.%fZ")
        mintime = dt.datetime.strptime(temp.Timestamp.min(), "%Y-%m-%dT%H:%M:%S.%fZ")
        d = {'purchased': session in buyersession,
             'day': maxtime.day,
             'month': maxtime.month,
             'navigations': len(temp.Timestamp)-1,
             'duration': int((maxtime-mintime).total_seconds()),
             'totalcats': len(temp.itemcategory.unique()),
             'totalitems': len(temp.itemid.unique()),
             'timesegment': segment[maxtime.hour],
             'segmentchange': segment[maxtime.hour] != segment[mintime.hour],
             }
        s = "{purchased}, {day}, {month}, {navigations}, {duration}, {totalcats}, {totalitems}, {timesegment}"
        f1.write(s.format(**d))

Lastly, the building of the words for the different times of day could be written slightly more concise when using a list instead of a dictionary:
segment = ['EM']*24
segment[7:10] = ['M']*3
segment[10:13] = ['A']*3
segment[13:18] = ['E']*5
segment[18:23] = ['N']*5
segment[23] = 'MN'

In the end your code is probably slow, because of all the finding of mins, maxs and lens. This just takes time. Would be good to see a run with a profiler to see at which operation, though.

With four more years of experience, here is how you could make this even faster:
def process(group):
    min_time = group.loc[group["Timestamp"].idxmin()]
    max_time = group.loc[group["Timestamp"].idxmax()]
    diff = max_time["Timestamp"] - min_time["Timestamp"]
    return pd.Series({"purchased": group["Sid"].iloc[0] in buyer_session,
                      "day": max_time["Timestamp"].day,
                      "month": max_time["Timestamp"].month,
                      "navigations": group["Timestamp"].count() - 1,
                      "duration": int(diff.total_seconds()),
                      "totalcats": group["itemcategory"].nunique(),
                      "totalitems": group["itemid"].nunique(),
                      "timesegment": max_time["segment"],
                      "segmentchange": max_time["segment"] != min_time["segment"]})

if __name__ == "__main__":
    buys_data = pd.read_csv(...)
    buyer_session = set(buys_data["Sid"].unique())
    clicks_data = pd.read_csv(...)
    clicks_data["Timestamp"] = pd.to_datetime(clicks_data["Timestamp"],
                                              format="%Y-%m-%dT%H:%M:%S.%fZ")
    clicks_data["segment"] = pd.cut(clicks_data["Timestamp"].dt.hour,
                                    [0, 7, 10, 13, 18, 23, 24],
                                    labels=["EM", "M", "A", "E", "N", "MN"])
    sessions = clicks_data.groupby("Sid", as_index=False).apply(process)
    sessions.to_csv('/tmp/gensim/yoochooseclick_new.dat', index=False)

I used the fact that you can directly get the number of unique items using nunique and that it is easier to group by the session ID than iterate over it using groupby (provided you are actually interested in all session IDs). I also converted to datetime objects right away and assigned the segements using cut. Before, there was a possible bug, because you determined the maximum and minimum date using string comparison. For the datetime format you have, this just happens to also sort correctly by date, but if that format ever changes, your code would break.


Implementing logic in a different way I need to write a function which will do the following functionalities

I would get a string and a array as input parameters

The string would be like one of the following for example

catalog
level1Cats
level2Cats

The String array would be containing the data for the strings mentioned above for example

catalog:("12605")

level1Cats:("12605_For the Home") OR level1Cats:("12605_Clothing")

level2Cats:("12605_For the Home_Appliances")

Now if the String is catalog and the array is the one mentioned above then the output should be 12605.

If its is level1Cats then the output should be an array of the following strings

12605_For the Home
12605_Clothing

I wrote the following code to implement the above mentioned logic It's doing the functionality without a flaw, Can i implement the same in a different and a more efficient way?

public static String[] parseFQ(String fqField, String fqs[]) {

    List<String> prefixes = new ArrayList<String>();

    if(fqField != null && fqField.length() > 0
            && fqs != null && fqs.length > 0) {

        //Get the fq which starts with given field name.
        for(String fq : fqs) {
            if(fq.startsWith(fqField)) {
                fqField = fq;
                break;
            }
        }

        boolean parsed = false;
        while(!parsed) {
            int quoteStart = fqField.indexOf("\"");
            int quoteEnd = (quoteStart >= 0) ? fqField.indexOf("\"", quoteStart+1) : -1;
            if(quoteEnd > 0) {
                prefixes.add(fqField.substring(quoteStart+1, quoteEnd));
                fqField = fqField.substring(quoteEnd+1, fqField.length());
            } else {
                parsed = true;
            }
        }
    }
    //Return the prefixes array
    return prefixes.toArray(new String[]{});
}

Below code is more readable and no need of using our logic around single quotes.

String[] w = fqField.split("\"");
for (int i = 1; i < w.length; i+=2) {
    prefixes.add(w[i]);
}


Convert boolean array to integer in Typescript I have a database object that has a field called workdays, it's a array boolean that contains 7 elements representing whether the user is active on the corresponding day.
I need to save this array into the database, I thought the bets way would be to convert the boolean array into a integer (bits) array and convert that into an integer which can be saved in the DB.
this is what I came up with, but I feel like I'm doing it the unnecessarily long way.
How can this be improved upon ?
    function serializeDays(days: boolean[]): number {
        // use (days || []) to avoid null pointer exception
        // concat it with Array(7).fill(false) to fill in missing values
        const d1 = (days || []).concat(Array(7).fill(false)).slice(0, 7);
        // map the boolean values to int values
        const d2 = d1.map(d => (d ? 1 : 0));
        // join the array to string
        const d3 = d2.join("");
        // parse the string into base 10 from base 2
        const n = parseInt(d3, 2);
        return n;
    }

    function parseDays(days: number): boolean[] {
        // use (days || 0) to avoid null pointer exception
        // convert the base 10 integer into base 2
        const d1 = (days || 0).toString(2);
        // split the string into an array
        // reverse it, concat it with Array(7).fill(0), reverse it again
        // this is just to zerofill the array from the left side
        const d2 = d1.split("").reverse().concat(Array(7).fill(0)).slice(0, 7).reverse();
        // parse the values from strings to actual integers to boolean
        const d3 = d2.map(d => (parseInt(d, 10) ? true : false));
        return d3;
    }

    // monday to friday
    serializeDays([true, true, true, true, true, false, false]); // outputs 124
    parseDays(124); // outputs [true, true, true, true, true, false, false]

```

Binary Math
Using string to do integer maths is very slow. You can use bitwise operators to encode and decode bit fields.
Encode bool array to int
The first function sets the low bit depending on the boolean. Each iteration the bits set are shifted to the left. The result is that the first array item is the highest bit (bit 7) and the last day is the lowest bit (bit 1)
Decode int to bool array
The second uses bitwise AND & to mask out all but the bit position required and adds a boolean to an array depending on the state of the bit.

// high bit is first item in days eg 0b1000000 is day[0]
function serializeDays(days) {
     return days.reduce((bin, day) => (bin << 1) + (!!day), 0);
}

function parseDays(bitField) {
     var bit = 7, days = [];
     while (bit) { days.push((bitField & (1 << --bit)) !== 0) }
     return days;
}

console.log(serializeDays([true,false,true,false,true,false,true]).toString(2));
console.log(parseDays(0b1010101).join(","));


HTTPUrlConnection code optimization I have this below code where i am sending some content over a POST and then consuming the response. I would like to know if there can be any optimizations that can be done and is there any possibility of connection leak.

import java.io.*;
import java.net.HttpURLConnection;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.util.Calendar;

class Test implements Serializable {

    static String getR(String xml, String requestUrl, int retryWaitTime, int socketTimeOut) throws IOException {
        long startTime = Calendar.getInstance().getTimeInMillis();
        StringBuilder xmlString = new StringBuilder();
        String out;
        HttpURLConnection connection = null;
        try {

            URL url = new URL(requestUrl);
            connection = (HttpURLConnection) url.openConnection();
            connection.setConnectTimeout(retryWaitTime);
            connection.setReadTimeout(socketTimeOut);
            connection.setDoInput(true);
            connection.setDoOutput(true);
            connection.setRequestMethod("POST");
            connection.setRequestProperty("Accept", "application/xml");
            connection.setRequestProperty("Content-Type", "application/xml; charset=UTF-8");
            OutputStreamWriter writer = new OutputStreamWriter(connection.getOutputStream(), StandardCharsets.UTF_8);
            writer.write(xml);
            writer.close();

            int statusCode = connection.getResponseCode();
            if (statusCode == HttpURLConnection.HTTP_OK) {
                BufferedReader br = new BufferedReader(new InputStreamReader(connection.getInputStream()));

                String line;
                while ((line = br.readLine()) != null) {
                    xmlString.append(line);
                }

                br.close();

               out = xmlString.toString();

            } else {

                out = statusCode + "^" + xml;
            }
            if (connection.getInputStream() != null)
                connection.getInputStream().close();

        }
        catch (Exception e) {

            out = "X^" + xml;
            assert connection != null;
            if (connection.getErrorStream() != null)
                connection.getErrorStream().close();
        }
        String Response = out.replace("\n", "").replace("\r", "");
        return Response;
    }

}

Star imports are discouraged. It's preferable to specify what classes you're importing in most cases.

getR is a very poor method name unless R is well defined inside the domain you're operating in. It's preferable to clearly express, without abbreviations, what the method will be doing. get is also misleading because it implies a simple local operation to retrieve a value from a class, not a remote call.

When variables will not be reassigned, it's preferable to mark them as final. This expresses design intent and tells the reader they don't have to worry about the value of the variable changing.

Minimizes scope wherever possible. getR should probably be either private or public. Package-private seems unlikely to be correct.

startTime is never used and can be removed. Rather than creating a Calendar instance, System.currentTimeMillis() would be preferable to find the current time.

If getR needs a URL, it should probably take it as a parameter, rather than accepting a String. In particular, your error handling of a bad URL is highly dubious. The caller gets no notification of any type that the URL string was incorrect. Make them build a correct URL and handle the case where they can't before the getR call.

There's an argument to be made that setDoInput(true) is noise, and one to be made that it makes things clearer.

The only exception your method throws is if the error stream can't be closed. It's unclear what you expect the caller to be able to do about that.

Use try-with-resources where possible to leverage automatic closing of resources. Otherwise you need to always close resources in finally blocks or you risk exceptions preventing you from closing them.

Always use curly braces, even if they're not required. You prevent an annoying and difficult-to-find class of error, and you enhance readability in most cases.

In idiomatic java, catch belongs on the same line as }.

Always catch the most specific type of exception possible. Catching Exception may silently swallow current or future exceptions you didn't intend to handle.

Using assert to ensure invariants in non-production environments is a good practice, but note that assertions must be engaged at both compile time and run time to function. Most production environments won't (and shouldn't) have assertions enabled. Many development environments and compilations also won't have assertions enabled, so make sure that they're correctly activated if you intend to use them.

It's unclear to me why you're explicitly closing the error stream in that fashion, since you never need or use it otherwise. If you must close it, put it in a try-with-resources block.

You should almost certainly log the exception you're handling for debugging later.

Early returns can clarify the code. The only one return location paradigm is a holdover from when it was not possible to embed returns in the middle of a block of code. A helper method could do the replacements for you.

In idiomatic java, variables start with a lowercase letter. return would be preferable to Return.

Turning the if check into a guard clause would reduce indentation and simplify the logic.

As of Java 9, I believe (but you should confirm) that string concatenation is actually faster than using a StringBuilder in the simple case of appending strings to one another. I think the readability is a wash.

If you were to make all these changes, your code might look more like:

private static String getR(
        final String xml,
        final URL requestUrl,
        final int retryWaitTime,
        final int socketTimeOut) {

    final StringBuilder xmlString = new StringBuilder();
    HttpURLConnection connection = null;

    try {
        connection = (HttpURLConnection) requestUrl.openConnection();
        connection.setConnectTimeout(retryWaitTime);
        connection.setReadTimeout(socketTimeOut);
        connection.setDoInput(true);
        connection.setDoOutput(true);
        connection.setRequestMethod("POST");
        connection.setRequestProperty("Accept", "application/xml");
        connection.setRequestProperty("Content-Type", "application/xml; charset=UTF-8");

        try (final OutputStreamWriter writer =
                new OutputStreamWriter(connection.getOutputStream(), StandardCharsets.UTF_8)) {
            writer.write(xml);
        }

        final int statusCode = connection.getResponseCode();
        if (statusCode != HttpURLConnection.HTTP_OK) {
            return sanitize(statusCode + "^" + xml);
        }

        try (final BufferedReader br = new BufferedReader(new InputStreamReader(connection.getInputStream()))) {
            String line;
            while ((line = br.readLine()) != null) {
                xmlString.append(line);
            }
        }
        return sanitize(xmlString.toString());

    } catch (final IOException e) {
        e.printStackTrace();
        return sanitize("X^" + xml);
    }
}

private static String sanitize(final String string) {
    return string.replace("\n", "").replace("\r", "");
}


Creating multiple DOM buttons in JavaScript (I am not DRYing) I am making a Tic-tac-toe game and I have the basic board but I am repeating the code and it is very badly written. I am wondering how I can use better practices to write the DOM elements that I need.

I am referring to the function boardController.buildBoard() which can be found here:

https://github.com/robbiesoho/TTTJS/blob/master/assets/js/script.js

There are nine buttons and here are the first two

const boxOne = document.createElement('div');
    boxOne.classList.add('boxOne');
    const buttonOne = document.createElement('button')
    buttonOne.classList.add("button")
    buttonOne.setAttribute('type', 'button');
    buttonOne.addEventListener('click', () => {
      boxOne.classList.add('token');
      boxOne.textContent =  playerController.activePlayer;
      boardController.board[0] =  playerController.activePlayer;

      gameController.afterButtonIsPressed();
      gameController.winnerAction;

    });

    const boxTwo = document.createElement('div');
    boxTwo.classList.add('boxTwo');
    const buttonTwo = document.createElement('button')
    buttonTwo.classList.add("button")
    buttonTwo.setAttribute('type', 'button');
    buttonTwo.addEventListener('click', () => {
      boxTwo.classList.add('token');
      boxTwo.textContent =  playerController.activePlayer;
      boardController.board[1] =  playerController.activePlayer;

      gameController.afterButtonIsPressed();
      gameController.winnerAction;

    });

I am new to JS and know only enough to know that this is not a good way to write this. Thank you in advance

Factor repeated code into a function

Any code that is all part of "doing one thing" can be naturally captured in a function definition and function call. Any details that differ between the two situations can be made into arguments to the function.

const boxOne = createBox( 'boxOne' );
const buttonOne = createButton( boxOne, 0 );
const boxTwo = createBox( 'boxTwo' );
const buttonTwo = createButton( boxTwo, 1 );

function createBox( name ){
  var box = document.createElement('div');
  box.classList.add( name );
  return box;
}
function createButton( box, idx ){
  var button = document.createElement('button')
  button.classList.add("button")
  button.setAttribute('type', 'button');
  button.addEventListener('click', () => {
    box.classList.add('token');
    box.textContent =  playerController.activePlayer;
    boardController.board[idx] =  playerController.activePlayer;

    gameController.afterButtonIsPressed();
    gameController.winnerAction;
  });
  return button;
}

This doesn't just apply to code that is repeated. Any block of code where you find yourself writing a comment about what it is doing could probably be expressed as a function. The function's name can express the idea of the comment. And by removing the bulk of code to a different location, it can make the main-line code easier (faster) to read. In the code here for example, the two lines

    gameController.afterButtonIsPressed();
    gameController.winnerAction;

would make for a nice small function. And a single line call to it probably wouldn't need to be set off by a paragraph break.


How can I improve the design of this serialization mechanism? I'm working on caching library which is intended to work with all types of values.

Values are represented as instances of value_type: value_type holds a std::vector<char> for the raw data, and a uint32_t flags integer value which is used to indicate the format of the stored data.

I would like to make it easy for the users of the library and provided a serialize() method that would convert a C++ type to a value_type so that instead of writing:

// In the line below, serialization::string is an integer constant
value_type value(serialization::string, "My string");

People could just write:

value_type value = serialize("My string");

And somehow the serialize() method would "guess" that the associated integer flag is serialization::string.

I would also make it possible for an user to register his own types and conversions into the serialization mechanism.

I came up with the following code:

/**
 * \brief A default serializer type.
 * \tparam Type The type to serialize.
 */
template <typename Type>
struct serializer {};

/**
 * \brief The base serializer type.
 * \tparam Type The serialized type.
 * \tparam Flags The flags.
 *
 * Derive from this type to enable a new serialization.
 */
template <typename Type, value_type::flags_type Flags>
struct base_serializer
{
    /**
     * \brief The type supported by the serializer.
     */
    typedef Type type;

    /**
     * \brief The flags.
     */
    static const value_type::flags_type flags = Flags;

    static value_type serialize(const type& value)
    {
        return value_type(flags, serializer<type>::serialize_data(value));
    }

    /**
     * \brief Deserialize a value.
     * \param value The value.
     * \return The deserialized value.
     */
    static Type deserialize(const value_type& value)
    {
        if (value.flags() != flags)
        {
            throw boost::system::system_error(serialization::error::invalid_types, serialization::serialization_category(), "deserialization failed");
        }

        return serializer<type>::deserialize_data(value.data());
    }
};

/**
 * \brief Serialize a value.
 * \tparam Type The type.
 * \param value The value.
 * \return The serialized value.
 */
template <typename Type>
value_type serialize(const Type& value) { return serializer<Type>::serialize(value); }

/**
 * \brief Deserialize a value.
 * \tparam Type The type.
 * \param value The value.
 * \return The deserialized value.
 */
template <typename Type>
Type deserialize(const value_type& value) { return serializer<Type>::deserialize(value); }

/**
 * \brief A const char* serializer.
 */
template <>
struct serializer<const char*> : public base_serializer<const char*, serialization::string>
{
    static std::vector<char> serialize_data(type value)
    {
        return std::vector<char>(value, value + strlen(value));
    }
};

/**
 * \brief An overload.
 * \param value A C-string.
 * \return The serialized value.
 */
value_type serialize(const char* value) { return serializer<const char*>::serialize(value); }

/**
 * \brief A std::string serializer.
 */
template <>
struct serializer<std::string> : public base_serializer<std::string, serialization::string>
{
    static std::vector<char> serialize_data(type value)
    {
        return std::vector<char>(value.begin(), value.end());
    }

    static type deserialize_data(const std::vector<char>& value)
    {
        return type(value.begin(), value.end());
    }
};

Do you guys see any improvement ?

Thank you.

You can use an internal traits type to reduce redundancy and complexity.

template< typename serialized >
struct serialize_traits; // no implementation of general case

enum class serialize_style {
    string,
    integral
};

typedef std::vector< char > serial_type; // consider std::string instead

template<>
struct serialize_traits< char * > {
    static const serialize_style style = serialize_style::string;
};

template<>
struct serialize_traits< std::string > {
    static const serialize_style style = serialize_style::string;
};

// Since serializer has no state, I broke this into a free function.
/* It works with both char * and std::string, but uses std::enable_if
   to hide itself unless you specify in serialize_traits that it will work. */

template < typename serialized >
typename std::enable_if< serialize_traits< serialized >::style == serialize_style::string,
serial_type >::type
serialize( serialized const &in ) {
    using std::begin;
    using std::end;
    return serial_type( begin( in ), end( in ) );
}

I need to go so I'll post this now… not tested.


Translating method calls to REST calls I understand and apply the DRY principle but I have a hard time getting this DRYer than it is right now.

/**
 * The Caller class is used to translate method calls to rest calls.
 *
 * It uses a low level AsyncHttpClient to execute the actual calls.
 *
 * It uses callbacks which are executed when the response is received.
 */

public class Caller {

    // the (rest) base url to where the calls must be directed
    private static final String baseUrl = "http://very.secret.domain.org:1337/";

    // singleton
    private static final Caller instance = new Caller();

    // the client which does the actual (low level) calls
    private static AsyncHttpClient client = new SyncHttpClient();

    // the (rest) method part of the url
    private static final String registerUrl = baseUrl + "createUser/";
    private static final String verifyUrl = baseUrl + "searchUser/";

    // singleton
    private Caller() {
    }

    synchronized static Caller getInstance() {
        return instance;
    }

    // creates a user with given parameters
    synchronized void getUser(final ShareActivity activity, // the activity which calls this method
                                                            // and expects the result (asynchronously)
                              String idToken, String name, String email) {

        //arguments
        Map map = new HashMap();

        map.put("token", idToken);
        map.put("name", name);
        map.put("email", email);

        //casted
        RequestParams params = new RequestParams(map);

        // call to get
        get(registerUrl,
                params,
                new JsonHttpResponseHandler() {//the handler

                    // when response recieved
            @Override
            public void onSuccess(int statusCode, Header[] headers, JSONObject user) {

                // pass the (json) User object to the (calling and passed) activity
                activity.handleUser(user);
            }

            @Override
            public void onSuccess(int statusCode, Header[] headers, JSONArray array) {

                // should not happen (the rest endpoint doesn't produce arrays)
                throw new RuntimeException("array in stead of object");
            }
        });
    }

    // more or less the same as above (except for added comments)
    synchronized void getUser(final ShareActivity activity, final String idToken) {

        // the same map again with the same first (and here only) parameter
        // I really think this can be done DRYer
        Map map = new HashMap();

        map.put("token", idToken);

        RequestParams params = new RequestParams(map);

        // same call
        get(verifyUrl, params,
                new JsonHttpResponseHandler() { // other handler

            @Override
            public void onSuccess(int statusCode, Header[] headers, JSONObject user) {

                // this call (with different url,
                // more or less the same map
                // does other work in the same activity)
                // that's to much of same in the same sentence I suspect ;-)
                if(user!=null){
                    activity.handleUser(user);
                }else {
                    activity.registerUser();
                }
            }

            @Override
            public void onSuccess(int statusCode, Header[] headers, JSONArray array) {

                // shouldn't happen either
                throw new RuntimeException("array in stead of object");
            }
        });
    }

    // the lowlevel call
    private static void get(String url, RequestParams params, AsyncHttpResponseHandler responseHandler) {
        client.get(url, params, responseHandler);
    }
}

As can be seen, the getUser() is overloaded. There are two different use cases which differ in URL, the content of map and the handler.

The get method on the bottom is reused but I was wondering whether it is possible to get this code cleaner (or DRYer in any case...).

General Hints

Naming

Your code does not respect Java Naming Conventions. The constants (static final members) names should be ALL_UPPER_CASE.

Singelton Pattern

Do not use the Singelton pattern. It makes it hard to reuse the code using this one in general and to test (in UnitTests) in particular.

BTW: you did it wrong.

Comments

Comments should explain why the code is like it is.

Your comments simply lie.

Following your comments the first method in JsonHttpResponseHandler should throw the exception, not the second.

This problem is quite common in code under development. Someone changes the behavior of the code but does not adjust the comments, turning them into lies.

So just delete the comments.

Supporting DRY

Often you don't have pure duplicated code. In most cases you have similar looking code which you usually quite easily can change to equal code by simple refactorings and/or by applying design patterns.

This goes basically in 3 steps.

identify similar code
make similar code the same
remove the duplication

Since this process is expected not to change the behavior of your application you should better have UnitTest that fix the desired behavior so that you get immediate response when you broke something.

Identify similar code

The biggest block of duplication is the anonymous inner class of type JsonHttpResponseHandler which is created twice with the almost same content.

The only difference is the check for the user being null.

Make similar code the same

In this step you convert individual similar parts of your code to look exactly the same.
Avoid typing as much as possible and rely on your IDEs refactorings like extract to local variable, rename in file and alike.

If the execution context of the first JsonHttpResponseHandler implementation guarantees that user never references null then you could simply copy the null check to it to make the same as the second.

Skip the rest of the section if that applies.

If not you should refactor out the difference. You could do so  by introducing a new interface:

public class Caller {
  private interface ActivityCaller{
     void handle(ShareActivity activity,JSONObject user);
  }

Then you implement the interface with the respective behavior before creating the instance of JsonHttpResponseHandler:

// first version
RequestParams params = new RequestParams(map);
ActivityCaller activityCaller = (a,user)-> a.handleUser(user); // java8 Lambda Style, lambda parameter name 'a' must differ from the methods parameter name
get(registerUrl, params, new JsonHttpResponseHandler() {
  @Override
  public void onSuccess(int statusCode, Header[] headers, JSONObject user) {
     activityCaller.handleUser(activity,user);
  }
  // .. no changes after here

// second versionRequestParams params = new RequestParams(map);
ActivityCaller activityCaller = new ActivityCaller(){ // traditional annonymous inner class
 @Override
  public void handle(ShareActivity activity,JSONObject user){
    if(user!=null){
        activity.handleUser(user);
    }else {
        activity.registerUser();
    }
  }
}
get(registerUrl, params, new JsonHttpResponseHandler() {
  @Override
  public void onSuccess(int statusCode, Header[] headers, JSONObject user) {
     activityCaller.handleUser(activity,user);
  }
  // .. no changes after here

Remove the duplication

Now both anonymous inner class implementations of JsonHttpResponseHandler are exactly the same. Use your IDEs convert anonymous to inner class refactoring to replace both occurrences.

This creates a new inner class:

// assumed you copied the `null` check
public class Caller {
  private static class JsonHttpResponseHandlerImpl{
    private final ShareActivity activity; // if you copied the null check
    JsonHttpResponseHandlerImpl(ShareActivity activity){
      this.activity = activity;
    }
    @Override
    public void onSuccess(int statusCode, Header[] headers, JSONObject user) {
        if(user!=null){
            activity.handleUser(user);
        }else {
            activity.registerUser();
        }
    }
    @Override
    public void onSuccess(int statusCode, Header[] headers, JSONArray array) {
        throw new RuntimeException("array in stead of object");
    }
  }

and both of your methods change to:

   RequestParams params = new RequestParams(map);
   get(registerUrl, params, new JsonHttpResponseHandlerImpl(activity));

As a result you have even more common code in both methods which you could easily move to the common get method.

What's left in your getUser() methods it the creation of the map with different content and the implementations of the new ActivityCaller interface if you decided for this way.


Compare Sheet1 and Sheet2 for every match and then write them on Sheet3 I have the following code

Sub checkdata()
Application.Calculation = xlCalculationManual
Dim sheet1 As Worksheet
Set sheet1 = Sheets(1)
Dim sheet2 As Worksheet
Set sheet2 = Sheets(2)
Dim sheet3 As Worksheet
Set sheet3 = Sheets(3)
lr1 = sheet1.Cells(Rows.Count, 1).End(xlUp).Row
lr2 = sheet2.Cells(Rows.Count, 1).End(xlUp).Row
lr3 = sheet3.Cells(Rows.Count, 1).End(xlUp).Row
For i = 1 To lr1
DoEvents
    check = sheet1.Cells(i, 1).Value
    lr3 = lr3 + 1
    For k = 1 To 5
        sheet3.Cells(lr3, k).Value = sheet1.Cells(lr3, k).Value
    Next k
    For j = 1 To lr2
        If sheet2.Cells(j, 4).Value = check And sheet2.Cells(j, 5).Value = vbNullString Then
            sheet2.Cells(j, 9).Value = "x"
            lr3 = lr3 + 1
            For k = 1 To 3
               sheet3.Cells(lr3, k + 5).Value = sheet2.Cells(lr3, k).Value
            Next k
            Exit For
        End If
    Next j
    lr3 = lr3 + 1
Next i
Application.Calculation = xlCalculationAutomatic
End Sub

It goes like: For each line on sheet1, copy the information I need to sheet3, then check all lines on sheet2 and see if it is from the same thing (Column 1 on sheet1 and Column 4 in sheet2). If it matches and it wasnt used yet, mark as used and put the info on sheet3 too, offset to the right.

Most of the variables are placeholders. Lr means lastrow, and the for loops uses i, j and k

I just have a problem that it is too slow, because both Sheet1 and Sheet2 can have around 100k lines, making it check 100k*100k times, to a total of 10kkk. I know the DoEvents makes it even slower, but I need a way to unlock excel, so it doesn't get that "Not responding" state for too long.

The output may look kinda weird, but it's how I need it for other places that I can't touch yet.

Shouldn't this:

For k = 1 To 5
    sheet3.Cells(lr3, k).Value = sheet1.Cells(lr3, k).Value
Next k

be this (you're not copying from row i on Sheet1) ?

For k = 1 To 5
    sheet3.Cells(lr3, k).Value = sheet1.Cells(i, k).Value
Next k

Would be faster as:

sheet3.Cells(lr3, 1).Resize(1, 5).Value = sheet1.Cells(i, 1).Resize(1, 5).Value

EDIT: this should be faster overall. Removes the nested loop and instead uses a dictionary to create a lookup on the sheet2 values.

Sub checkdata()

    Application.Calculation = xlCalculationManual
    Dim sheet1 As Worksheet, sheet2 As Worksheet, sheet3 As Worksheet
    Dim dict As Object, v

    Set sheet1 = Sheets(1)
    Set sheet2 = Sheets(2)
    Set sheet3 = Sheets(3)

    lr1 = sheet1.Cells(Rows.Count, 1).End(xlUp).Row
    lr2 = sheet2.Cells(Rows.Count, 1).End(xlUp).Row
    lr3 = sheet3.Cells(Rows.Count, 1).End(xlUp).Row

    'Map all values+rows in Sheet2 ColD
    Set dict = CreateObject("scripting.dictionary")
    For j = 1 To lr2
        If sheet2.Cells(j, 5).Value = vbNullString Then
            v = jsheet2.Cells(j, 4).Value
            If Not dict.Exists(v) Then dict.Add v, j '<< capture value (first instance only) and row
        End If
    Next

    For i = 1 To lr1

        check = sheet1.Cells(i, 1).Value
        lr3 = lr3 + 1
        sheet3.Cells(lr3, 1).Resize(1, 5).Value = _
                sheet1.Cells(i, 1).Resize(1, 5).Value

        'does check exist on Sheet2 (with empty ColE value)?
        If dict.Exists(check) Then

            sheet2.Cells(dict(check), 9).Value = "x" '<<< not Col 5 ?
            lr3 = lr3 + 1 '<<< needed?
            sheet3.Cells(lr3, 6).Resize(1, 5).Value = _
                    sheet2.Cells(dict(check), 1).Resize(1, 5).Value
            Exit For
        End If

        lr3 = lr3 + 1 '<<< needed?
    Next i

    Application.Calculation = xlCalculationAutomatic

End Sub


Abort thread on new request, but complete only last request The situation is that a certain asynchronous operation is called multiple times, which is basically to consolidate results. But I want to abort every time a new request comes in, which means that only the last call will run completely. Does this solution look reasonable?

I have tried replicating the process in a test program, where the for loop represents 'x' number of calls generated for consolidation. But because we only need to run calculation once, we can safely run it on the last call.

I am creating a new thread each time, and cancelling the previous ones if there are any. This seems to work, but is there a better way or can this be improved?

static void Main(string[] args)
{
    var listCancellationTokens = new List<CancellationTokenSource>();

    for (var i = 1; i <= 5; i++)
    {
        var cancelSource = new CancellationTokenSource();
        listCancellationTokens.ForEach(c => c.Cancel());

        CreateThread(cancelSource);

        listCancellationTokens.Add(cancelSource);
    }
}

private static void CreateThread(CancellationTokenSource cancellationTokenSource)
{
    new Thread(() =>
    {
        try
        {
            ConsolidateResults(cancellationTokenSource.Token);
        }
        catch
        {
            ;
        }
    }).Start();

}

private static void ConsolidateResults(CancellationToken cancelToken)
{
    UpdateValues();
    if (cancelToken.IsCancellationRequested)
    {
        return;
    }
    TurnOffEvents();
    if (cancelToken.IsCancellationRequested)
    {
        return;
    }
    GetMarginPercentage();

    if (cancelToken.IsCancellationRequested)
    {
        return;
    }
    TurnOnEvents();
}

//This is how it is done at the moment
private void Commit(bool adjustDiscount = false)
{
    lock (_updateRunnerLock)
    {
        _updateRunnerRemainingCycles = 40;
        if (_updateRunner == null)
        {
            _updateRunner = new Thread(UpdateRunner);
            _updateRunner.Start();
        }
    }
}

private void UpdateRunner()
{
    while (_updateRunnerRemainingCycles > 0)
    {
        Thread.Sleep(500);
        _updateRunnerRemainingCycles--;
    }
    lock (this)
    {
        if (_updateRunner != null)
        {
            _updateRunner = null;
            ConsolidateResults();
        }
    }
}

Basically it is in its current state senseless to have a

var listCancellationTokens = new List<CancellationTokenSource>();

if all you want is one running thread at any time.

So what I suggest instead is to create one Task and if there comes the situation that this task should be canceled you should cancel the task and Wait until the task returned.

After the Task returned you then create the new Task.

private static Task task;
private static CancellationTokenSource cancelSource;
static void Main(string[] args)
{

    for (var i = 1; i <= 5; i++)
    {
        if(task != null && (!task.IsCompleted || !task.IsFaulted))
        {

            try
            {
                cancelSource.Cancel();
                task.Wait(cancelSource.Token);
            }
            catch (OperationCanceledException ex)
            {
                //swallow because by using ThrowIfCancellationRequested()
                //in combination with cancelSource.Cancel() an exception is thrown.
            }
        }
        cancelSource = new CancellationTokenSource();

        task = Task.Factory.StartNew(() => ConsolidateResults(cancelSource.Token));

    }
    Console.ReadLine();
}

then you should change the ConsolidateResults() method to

private static void ConsolidateResults(CancellationToken cancelToken)
{
    cancelToken.ThrowIfCancellationRequested();

    UpdateValues();
    TurnOffEvents();
    GetMarginPercentage();
    TurnOnEvents();
}


Remove duplicate values in array with one loop? I am writting a program to remove the duplicate values from an array, is any way to optimize code by removing one loop?

var arr=[10,20,30,20,40,10],
newarr=[];
var foo=function(ele, index, array){
     match=array[index];
  for(var m in newarr){
  if(newarr[m]==match)
    return true;
  }
  newarr.push(match);
};
arr.forEach(foo);
console.log(newarr);

Yes, there is.

Your current solution has a O(n²) runtime complexity which is worse then necessary.

By sorting the array and then only marching once, comparing only neighboring elements, extracting unique elements costs only O(n * log(n)).

var arr=[10,20,30,20,40,10];

function uniqueBySorting(array) {
  var sortedArray = array.slice().sort(function(a, b) {
    return a > b ? 1 : 0;
  });
  var result = [];

  result.push(sortedArray[0]);
  for (var i = 1, length = sortedArray.length; i < length; i++) {
    if (sortedArray[i] !== sortedArray[i - 1]) {
      result.push(sortedArray[i]);
    }
  }
  return result;
}

console.log(uniqueBySorting(arr));

However, that comes at the cost of loosing the sort order, which might not be desired. If the sort order matters, use a hashed set for remembering which elements you already encountered instead of the inner loop, so you finally end up with a O(n) complexity.

var arr=[10,20,30,20,40,10];

function uniqueWithObjectHashSet(array) {
  var hashSet = {}, result = [];
  array.forEach(function(element) {
    if (!element in hashSet) {
      newarr.push(element);
      hashSet[element] = true;
    }
  });
  return result;
}

console.log(uniqueWithObjectHashSet(arr));

Inserting elements into a hash set and looking them up requires only constant time, opposed to the linear time required for iterating over the target array.

Technically JS does not have any native hashset implementation, but lucky for you most JS engines tend to implement objects as hashsets internally (unless the JIT can identify properties with constant names), so you can abuse that behavior.

See Hashset in Javascript for more information on the last point.

Comparison of the HashSet implementation (including @hindmost's variant) to previous solutions: http://jsperf.com/cr-57581/7

Not surprising, all versions with nested loops are left far behind. As of April 2015, the hashSet version using Array.prototype.forEach is by far the fastest algorithm in Firefox. In Chrome with V8 however, the sorting based solution is much faster, most likely due to a more efficient implementation of Array.prototype.sort.


Is there a more efficient way of doing function 'which' in R? The data file is here.

Here are my codes:

> ftable=read.table(file.choose())
> start.time=Sys.time()
> 1-length(which(ftable==1))/sum(ftable)
[1] 0.12
> end.time=Sys.time()
> end.time-start.time
Time difference of 0.004880905 secs

I understand that 0.00488 secs are not a lot. But I have to repeatedly do this calculation over different and larger tables. I am wondering if the function 'which' can be replaced by a more efficient one.

Thanks in advance!

Note: This piece of codes is to calculate the percentage of singletons in ftable. If there is a more efficient way, please let me know. Thank you!

Try:

1-sum(ftable==1L)/sum(ftable)

Test on larger data:

n <- 1000000
set.seed(21)
ftable <- data.frame(replicate(3, sample.int(4, n, replace = T))-1L)

start.time=Sys.time()
1-length(which(ftable==1))/sum(ftable)
end.time=Sys.time()
end.time-start.time
# Time difference of 0.1981359 secs

start.time=Sys.time()
1-sum(ftable==1L)/sum(ftable)
end.time=Sys.time()
end.time-start.time
# Time difference of 0.06704712 secs

bechmarks:

n <- 1000000
set.seed(21)
ftable <- data.frame(replicate(3, sample.int(4, n, replace = T))-1L)

jz <- function() 1-length(which(ftable==1))/sum(ftable)
minem <- function() 1-sum(ftable==1L)/sum(ftable)

br <- bench::mark(jz(), minem(), iterations = 50)
br[, 1:7]
# A tibble: 2 x 7
#   expression      min     mean   median      max `itr/sec` mem_alloc
#   <chr>      <bch:tm> <bch:tm> <bch:tm> <bch:tm>     <dbl> <bch:byt>
# 1 jz()         51.2ms   53.8ms   52.6ms   66.3ms      18.6    60.1MB
# 2 minem()      37.7ms   39.9ms   38.5ms     67ms      25.1    45.8MB
# only around 36 % faster


Matrix class with lots of tiny methods I have been following the advice to make tiny methods that does just one thing and does it well. I also have been keen on reducing or eliminating duplication as much as possible.

But when a very experienced developer friend reviewed my code, he mentioned I was taking it too far. And, in short, my code was unreadable as it jumped too much killing the flow. Though I too feel I am pretty bad in naming my methods, I feel such tiny methods are helping me break down the problem and letting me solve it easily.

Challenge: Matrix Rotation (Move each matrix element to the neighboring position along concentric rectangular paths. The dimensions of the matrix are guaranteed to be even.)

Solution: matrix_rotator.rb

class Matrix
  def initialize(two_d_array: nil)
    @data = two_d_array
  end

  def rotate!(anti_clockwise: 0)
    rotated = layers
      .map { |layer| layer.map { |row, column| @data[row][column] } }
      .map { |layer| layer.rotate(anti_clockwise) }

    coordinates.zip(rotated.flatten).each do |(row, column), n|
      @data[row][column] = n
    end

    self
  end

  def to_s
    @data
      .map { |row| row.join(' ') }
      .join("\n")
  end

  def coordinates
    layers.each_with_object([]) { |layer, a| a.push(*layer) }
  end

  def height
    @height ||= @data.length
  end

  def width
    @width ||= @data.first.length
  end

  def layers
    number_of_layers.times.map do |index|
      top(index) + right(index) + bottom(index) + left(index)
    end
  end

  def horizontal_segment(index)
    (width - (2 * index) - 1).times
  end

  def vertical_segment(index)
    (height - (2 * index) - 1).times
  end

  def top(index)
    horizontal_segment(index)
      .map { |w| [index, w + index] }
  end

  def right(index)
    vertical_segment(index)
      .map { |h| [h + index, width - index - 1] }
  end

  def bottom(index)
    horizontal_segment(index)
      .map { |w| [height - index - 1, width - index - w - 1] }
  end

  def left(index)
    vertical_segment(index)
      .map { |h| [height - 1 - index - h, index] }
  end

  def number_of_layers
    [height, width].min / 2
  end
end

class MatrixRotator
  def initialize(source: nil)
    parse(source.readlines)
  end

  def result
    Matrix
      .new(two_d_array: two_d_array)
      .rotate!(anti_clockwise: @shift)
      .to_s
  end

  def parse(lines)
    parameters, *@content = lines.map(&:strip)
    @shift = parameters.split.map(&:to_i).last
  end

  def two_d_array
    @content.map { |line| line.split.map(&:to_i) }
  end
end

puts MatrixRotator.new(source: STDIN).result

The way you dissected the problem was a good way to get a handle on it but once you had, it reveals some refactoring opportunities.  Once you have the individual methods, you can begin to see how to build them back together

Let's begin by focusing on your two Matrix#<xxx>_segment methods.  Both have the same method signature and differ only by the use of width and height. So those could be combined to just be

def segment(index, length)   # maybe name it segment_enumerator just to be clear?
  (length - (2 * index) - 1).times
end

For top(index) then, you get

def top(index)
  segment(index, width)
    .map { |w| [index, w + index] }
end

which has the advantage of keeping width in the same function, making it slightly more readable and DRY.

Your side functions (top, right, et al.) really only differ by the block they use.  So you could write a single map_side method that just passes a block in the map.  However, because of the refactor we just did in the new #segment, a lambda helps pull it together:

def map_side(index, length, &block)
  lambda{ segment(index, length).map &block }
end

You might want to name that map_side something that means more to you in the problem domain (I know it returns an array of arrays, but didn't come up with a better name than map_side).

With that, we just use the new method to set up a quick hash of lambdas for each side, passing each the specific block you wanted:

def sides(index)
  _sides = {}
  _sides[:top] =    map_side(index, width)  { |w| [index, w + index] }
  _sides[:bottom] = map_side(index, width)  { |w| [height - index - 1, width - index - w - 1] }
  _sides[:left] =   map_side(index, height) { |h| [height - 1 - index - h, index] }
  _sides[:right] =  map_side(index, height) { |h| [h + index, width - index - 1] }
  _sides
end

Note, this is a hash of lambdas now, not the arrays you're looking for yet.  But with it, we can now write this for layers in whatever order you need:

def layers
  number_of_layers.times.map do |index|
    _sides = sides(index)
    [:top, :right, :bottom, :left].inject([]) do |array, side|
      array += _sides[side].call
    end
  end
end

A key thing to look at when you have tiny methods is whether you ended up using those methods more than once.  I like labeling things too to keep them straight (and that's always a Good Thing), but when it's just a single use, a variable name is as good as a method name.  So, we can just get rid of number_of_layers as a method.  I called it count below, but whatever works for you.

def layers
  count = [height, width].min / 2
  count.times.map do |index|
    _sides = sides(index)
    [:top, :right, :bottom, :left].inject([]) do |array, side|
      array += _sides[side].call
    end
  end
end

Now the following methods can be safely removed: horizontal_segment, vertical_segment, top, bottom, left, right, and number_of_layers.

The code is now DRYer, the width and height references are near each other for their specific uses, and you can quickly see how they stack up (in other words, your naming of elements was retained).  The one part that might throw you if you're not used to it is how I used the lambda closures to pull this off, but it's not an uncommon approach in Ruby and as you can see, a handy one.

I tested this refactor with your test suite and it ran fine.  I didn't benchmark, but it certainly didn't seem any slower and might be a little faster.


Controller with abstract methods for a delete method I'm studying OOP and MVC structure, and all my controllers extends the main controller, and almost all of them has a delete method, basically this:

public function delete () {

        if( isset($_POST) && !empty($_POST) ) {

            $Validator = new Validator;
            $Validator->validation_rules(array(
                    'id'    => 'required|numeric',
            ));

            $validated = $Validator->run($_POST, true);
            if($validated === false) {
                    echo helperModel::toJson(
                    array(
                        'success'=>0,
                        'message'=>$Validator->get_readable_errors(true)
                    ));
            } else {
                $delete_contact = (new contactModel)->delete($validated['id']);
                if($delete_contact){
                    echo helperModel::toJson(array('success'=>1));
                }
            }
        }

    }

I created a variable in the child controller with the name $default_model to refer to the controller model.

Example: in the userController I use the userModel, and copied the structure of the method to the main controller. Instead of using the specific name of the model, I just put:

(new $this->$default_model)->delete($validated['id'])

Is it a correct/recommended practice? If not, what should I do?

Three part answer here: a quick answer, a more detailed answer with alternatives, and some specifics to your code.

Quick Answer

There isn't anything wrong with having abstract controller methods, anymore than there is to having abstract methods in general.  That being said, the example you gave isn't an abstract method: it is simply a  parent method.  An abstract method is a method without a definition that must be defined by the child class.  That would be a bad idea in your use case, because there might be cases when you don't need to delete entries but would have to provide a definition anyway.  You are simply looking to implement some commonly used functionality in your parent class for easy-reuse, which is perfectly reasonable.  The fact that this is a controller doesn't change the overall picture at all, although there are a few caveats to keep in mind.

Finally, having the behavior of the parent method change based on definitions in the child method is perfectly reasonable to.  I use that pattern myself frequently (I'm referring to the definition of $default_model in the child class).  That could be considered an abstract property, except that PHP doesn't have such a thing.  As a result the only issue to watch out for there is the possibility that the developer that uses this system forgets to define the necessary properties.  There are two good ways to avoid that:

Instead of using a property, use an actual abstract method, which is supposed to return a string with the same value
Inside the parent method, check for a correctly defined $default_model and throw a detailed exception if it is missing: 'Cannot execute delete method for class ' . get_class($this) . ' because $default_model is not defined'  This way if you try to use this later and forget to declare all the properties, you get a clear and immediate warning telling you exactly what you did wrong.

Long Answer

Let's consider your goal: you have lots of controllers to implement lots of CRUD operations.  (for example) You need to CRUD users, you need to CRUD Customers, more CRUDs for Orders, products, categories, and so forth.  After putting together your second controller filled with repetitive CRUD methods, you really should be wondering "what can I do to avoid repeating myself so much" which is what you are really asking here.  You are trying to fix this problem by putting some basic CRUD functionality into the parent.  As I said, that is a reasonable approach and can avoid a lot of repetition.  It's worth mentioning the potential downsides:

This is a non-starter if your framework automatically routes to public controller methods.  This isn't the norm, but I've seen frameworks that do it.  If your public controller methods are automatically valid routes in your system, then putting any public methods in a base class means that every controller that extends that base has all those routes by default.  This is a very easy way to introduce accidental security vulnerabilities.
Your current implementation will also cause you trouble if you ever need flexibility in how the delete method is called.  I like the old adage "The only thing worse than no abstraction is the wrong abstraction".  For instance, what if you have a public-facing user's page and the user should only be able to delete themselves?  You won't be able to use this guy.  The validation rules are fixed, the calling sequence is fixed, etc...  Baking in a little more flexibility may be good, once you know what areas you need flexibility for.
Although less of a problem, this can result in unnecessary code "living" in your controllers if you put it in the base but don't actually need it all the time.

I consider that first one to be a show-stopper, but the second one is one you can plan for, and the third one is not a big deal, so (again), what you are doing isn't crazy.  There are a couple other ways to do this though, just because:

You can use traits, and have a trait for each CRUD method.  This way you only have to throw in the ones that you want.
You could put the CRUD functionality in separate classes that get generated and called as needed.  You could imagine having a deleteManager class which is passed in a model name (default_model) and the $_POST input, and which then does everything you have in your example.  This will again give you more flexibility.  Your controller only contains the functionality it needs, and you can have as many variations as you need without cluttering up your base class.  Maybe you have one deleteManager that does exactly what you have here, another that extends it and adds some authentication checks, etc... Maybe not the best examples but you should get the idea.

A review of your code

This is code review, and while I certainly want to answer your immediate question, a big goal of this site is to review everything.  So if you don't mind me making some other comments:

Your indentation is off.  You indent 8 spaces after the first line, and four spaces elsewhere.  I'm sure it was just a mistake, and it is best to try to fix those things before posting here, but it is more important to fix it in your code.  Following code formatting best practices makes reading code much easier, which makes debugging code much easier, which makes coding much easier.  You'd be surprised how often the little things matter.
You have a lot of inconsistencies in how you stylize your variable names and method names.  Choose snake_case, camelCase, or TitleCase and stick to it.  You use all three: Validator, validation_rules, toJson.  If uncertain just use PSR.
Your code doesn't properly addressing all possible branches.  No action is taken if the data validates but the delete fails.  This may cause problems for whatever client is calling this API endpoint.
You aren't using dependency injection.  That isn't exactly something that you fix overnight, but maintaining any size code base without automated tests (i.e. unit and integration testing) is a gigantic pain, and doing that requires dependency injection.  It makes for better code anyway.  Now is the time to start learning it.
I think there is some real code smell here: helperModel::toJson.  What is this helperModel and why does it have the job of converting to JSON?  Sounds suspiciously like a God Class.
I think some of your variables could be named better.  Granted, naming variables is hard, but it is worth the effort.  For instance, $default_model.  That doesn't give much hint as to what it is, and presumably the variable itself doesn't contain the model but rather the name of the model.  model_name might be better.  Similarly delete_contact is probably better off as a simple contact.  It is going to be deleted but it is still just a contact.  That one might be a nitpick though.
Finally, return early (aka guards).

I've loosely pulled together most of these suggestions for a counter-example:

public function delete(){
    if ($_SERVER['REQUEST_METHOD'] != 'POST'){
        echo 'error';
        return;
    }

    if(empty($_POST)){
        echo 'error';
        return;
    }

    // since the validator can't do anything without validation rules,
    // I would just pass them in to the constructor rather than
    // calling a separate method.
    $validator = new validator(array(
        'id'    => 'required|numeric',
    ));

    $validated = $validator->validate($_POST, true);
    if($validated === false){
        echo 'error';
        return;
    }

    $modeClass = $this->modeClass;
    $model = new $modelClass;
    if (!$model->delete($validated['id'])){
        echo 'error';
        return;
    }

    echo 'success';
}


Best practice javascript objects I am basically looking for Critique on the following code as i know its not the best and i am trying to find a set way of building javascript objects.

Be brutal as you way its a learning tool for me, basically i am pulling in a twitter feed via json and adding it to the dom.

var TwitterTimeline = {
  twitterFeed: function (data) {
    $.ajax({
      url: 'http://example.com/api/twitter/timeline/name/' + data.name,
      dataType: 'json',
      crossDomain: true,
      cache: false,
      success: function (items) {
        var tweets = "
";
        $.each(items, function (i, tweet) {
          var parser = new TwitterTimeline.Parser(tweet.text);
          parser.linkifyURLs();
          parser.linkifyHashTags();
          tweets += "

" + tweet.user.name + "
" + parser.getHTML() + "
";
          if(i === (data.limit - 1)) {
            return false;
          }
        });
        tweets += "
";
        $(data.id).html(tweets);
      }
    });
  },
  Parser: function (text) {

    var html = text;

    var urlRegex = /((ftp|http|https):\/\/(\w+:{0,1}\w*@)?(\S+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?)/gi;
    var hashTagRegex = /#([^ ]+)/gi;

    this.linkifyURLs = function () {
      html = html.replace(urlRegex, '$1');
    };
    this.linkifyHashTags = function () {
      html = html.replace(hashTagRegex, '#$1');
    };

    this.getHTML = function () {
      return html;
    };

  }
}

TwitterTimeline.twitterFeed({
  id: "#twitter-feed",
  name: 'testaccount',
  limit: 2
});

And my html

Any issues or bad practise which i bet there is a lot please let me know. So many different way to do things with javascript i always feel like i am hacking things together without any structure.

Thanks

//Let's wrap this module in a closure so we can have a private scope for our
 //module. That way, we don't expose too many globals as well as place too
 //many things in our namespace
;
(function (exports) {

  //Let's pull out these guys since they are static, and we don't want them
  //get recreated everytime a parse is required.
  var urlRegex = /((ftp|http|https):\/\/(\w+:{0,1}\w*@)?(\S+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?)/gi;
  var hashTagRegex = /#([^ ]+)/gi;

  //I suggest you use a templating system. In this example, I am using
  //Mustache. As you can see, it's easily readable, where you have an
  //array of tweets, each printing the contained section.
  var feedTemplate = '
{{#tweets}}

{{user.name}}
{{parsedHTML}}{{/tweets}}
';

  //Now our parser doesn't need to be an object. It could only be a function
  //that returns parsed data. Also, unless parse is required to be public, it
  //should remain inside the scope that requires it. That way, we don't
  //expose too much to our namespace.

  function parse(html) {
    return html.replace(urlRegex, '$1')
               .replace(hashTagRegex,'#$1');
  }

  //We move out this function to separate it from the AJAX call. That way,
  //it's not jumbled up with the AJAX code.

  function format(data, config) {

    //We have an array of twets that we will use later with the templating.
    var tweets = [];

    $.each(data, function (i, tweet) {
      //Now, for each tweet retrieved, we parse the text, and reattach it to
      //the tweet object as parsedHTML, which will be used in the template.
      tweet.parsedHTML = parse(tweet.text);

      //We then collect the tweets into the array defined earlier.
      tweets.push(tweet);

      //We needed that array, so that we won't feed the original array into
      //the template. So here's the break when the limit is reached.
      if(i === config.limit) return false;
    });

    //Now, with the template, and our tweet array, we render. The return is
    //an HTML string, which we can then use jQuery to build the elements.
    var html = Mustache.render(feedTemplate, {
      tweets: tweets
    });

    $(data.id).html(html);
  }

  //We expose our twitterFeed function to our namespace
  exports.twitterFeed = function (config) {
    $.ajax({
      url: 'http://example.com/api/twitter/timeline/name/' + data.name,
      dataType: 'json',
      crossDomain: true,
      cache: false,
      success: function (data) {
        format(data, config);
      }
    });
  }

}(this.TwitterTimeline = this.TwitterTimeline || {}));

Minus the comments, this is how it looks:

;(function (exports) {
  var urlRegex = /((ftp|http|https):\/\/(\w+:{0,1}\w*@)?(\S+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?)/gi;
  var hashTagRegex = /#([^ ]+)/gi;
  var feedTemplate = '
{{#tweets}}

{{user.name}}
{{parsedHTML}}{{/tweets}}
';

  function parse(html) {
    return html.replace(urlRegex, '$1')
               .replace(hashTagRegex,'#$1')
  }

  function format(data, config) {
    var tweets = [];
    $.each(data, function (i, tweet) {
      tweet.parsedHTML = parse(tweet.text);
      tweets.push(tweet);
      if(i === config.limit) return false
    });
    var html = Mustache.render(feedTemplate, {
      tweets: tweets
    });
    $(data.id).html(html)
  }

  exports.twitterFeed = function (config) {
    $.ajax({
      url: 'http://example.com/api/twitter/timeline/name/' + data.name,
      dataType: 'json',
      crossDomain: true,
      cache: false,
      success: function (data) {
        format(data, config)
      }
    })
  }

}(this.TwitterTimeline = this.TwitterTimeline || {}));


Two implementations of a PHP sign-up page I want the most secure and well-written version for my sign-up page. I'm adding AJAX to it for validation, too. I'm still writing it.

If you have tips on AJAX or on how to improve this PHP code for AJAX, feel free to point out any mistake and make any modification you want, or if there are other security concerns I should worry about. I'm fairly new this is the most I know about how to fight SQL injection.

I wrote this version first after I learned about SQL injection:

if (isset($_POST['submit'])){
include_once 'dbh.inc.php';
  $username = mysqli_real_escape_string($conn, $_POST['username']);
  $pwd = mysqli_real_escape_string($conn, $_POST['pwd']);
  $email = mysqli_real_escape_string($conn, $_POST['email']);
if(empty($username) || empty($pwd) || empty($email)){
  echo "Fill in all Fields!";
}else {
   if(!filter_var($email, FILTER_VALIDATE_EMAIL)){
     echo "Email is invalid!";
   }else{
      $stmt = $conn->prepare("SELECT * FROM users WHERE username=?");
      $stmt->bind_param("s", $uid);
      $uid = $username;
      $stmt->execute();
      $result = $stmt->get_result();
      $usernamecheck = mysqli_num_rows($result);
      $rowNum = $result->num_rows;
    if($rowNum > 0){
        echo "Username is taken!";
    }else{
        $stmt = $conn->prepare("SELECT * FROM users WHERE email=?");
        $stmt->bind_param("s", $uemail);
        $uemail = $email;
        $stmt->execute();
        $result = $stmt->get_result();
        $usernamecheck = mysqli_num_rows($result);
        $rowNum = $result->num_rows;
    if($rowNum > 0){
        echo "Email is taken";
    }else{
        $hashedPwd = password_hash($pwd, PASSWORD_DEFAULT);
        $stmt = $conn->prepare("INSERT INTO users (username, pwd, email) VALUES (?, ?, ?)");
    $stmt->bind_param("sss",$uid, $password, $theemail);
    $uid = $username;
    $password = $hashedPwd;
    $theemail= $email;
    $stmt->execute();
    $result = $stmt->get_result();
    header("location: ../user-login.php");
        }
        }
     }
    }
 }else{
header("location: ../user-signup.php");
exit();
}

and this version, which I was told is a better version, but has no prepared statements, which i learned is the main thing to fight SQL injection:

include_once '../dbh.inc.php';
if ($_SERVER['REQUEST_METHOD'] === 'POST') {
$username = $_POST['username'];
$email = $_POST['email'];
$pwd = $_POST['pwd'];
if(empty($username) || empty($pwd) || empty($email)){
  echo "Fill in all Fields!";
  exit();
}elseif(!filter_var($email, FILTER_VALIDATE_EMAIL)){
     echo "Email is invalid!";
     exit();
   }else{
$chk = "SELECT * FROM user WHERE email='$email'";
$result = mysqli_query($conn, $chk);
while ($row = mysqli_fetch_array($result)) {
  if ($row['email'] == $email) {
    echo "Email Match";
  exit();
  } else {
   $chk = "SELECT * FROM user WHERE username='$username'";
   $result = mysqli_query($conn, $chk);
   while ($row = mysqli_fetch_array($result)) {
    if ($row['username'] == $username) {
    echo "Username Match";
    exit();
  } else {
$hashedPwd = password_hash($pwd, PASSWORD_DEFAULT);
$join = sprintf("INSERT INTO user(username, email, password) " .
    "VALUES ('%s', '%s', '%s', '%s'); ",
  mysqli_real_escape_string($conn, $username),
  mysqli_real_escape_string($conn, $email),
  mysqli_real_escape_string($conn, $hashedPwd),
  mysqli_insert_id($conn));
  if (mysqli_query($conn, $join)) {
    $i = "joined";
  } else {
    $i = "failed to join";
  }
  echo $i;
  }

Escape the hashed password, don't hash the escaped password.... just hash the pw.
Prefer prepared statements over escaping.
Break up the logic into functions.
Please indent better - your formatting is a bit of a mess.
You do not need to use both escaping and prepared statements. Pick one. (Pick prepared statements)

Edit:

In this case, if your IDs are integers, you don't need to escape them or use prepared statements. Just check if they are integers first with is_numeric(). If you know they're integers then you know they're safe.

Generally though, prepared statements are the way to go.


Passing input parameters correctly to a method Below is my Interface:

public interface IDBClient {
    public String read(ClientInput input);
}

This is my implementation of the interface:

public class DatabaseClient implements IDBClient {

    @Override
    public String read(ClientInput input) {

    }
}

Now I have a factory which gets the instance of DatabaseClient like this:

IDBClient client = DatabaseClientFactory.getInstance();
....

Now I need to make a call to read method of my DatabaseClient which accepts the ClientInput parameter and below is the class for the same. This class was not written by me so that is the reason I am having a question on this and I am pretty much sure this is the wrong way of doing it.

public final class ClientInput {

    private Long userid;
    private Long clientid;
    private Long timeout_ms = 20L;
    private boolean debug;
    private Map<String, String> parameterMap;

    public ClientInput(Long userid, Long clientid, Map<String, String> parameterMap, Long timeout_ms, boolean debug) {
        this.userid = userid;
        this.clientid = clientid;
        this.parameterMap = parameterMap;
        this.timeout_ms = timeout_ms;
        this.debug = debug;
    }
}

So when customer make a call to read method of DatabaseClient, they will create the ClientInput parameter like this and then use the factory to get the Instance of DatabaseClient and then call the read method accordingly.

Map<String, String> paramMap = new HashMap<String, String>();
paramMap.put("attribute", "segmentation");

ClientInput input = new ClientInput(109739281L, 20L, paramMap, 1000L, true);

IDBClient client = DatabaseClientFactory.getInstance();
client.read(input);

Is this the right way of creating the ClientInput and then passing it to the read method? If not, then what's the best way of doing it? Where should I be doing validation check to see whether the customer has passed valid user IDs or valid client ID, etc.?

Like most times, it is best to validate your data before you use it. In your case, since the input data needs to be validated by the Database infrastructure, I recommend adding it to the IDBClient interface.

I also assume that the paramMap is the query you want to run, and not part of the user/login information.

I suspect the logical thing to do is to extend your interface to look like:

public interface IDBClient {

    public interface SessionHandle extends AutoCloseable, Closeable {
        public boolean isAlive();
    }

    public SessionHandle createSession(Long userid, Long clientid,
              Long timeout_ms, boolean debug) throws InvalidLoginException;

    public String read(SessionHandle session, Map<String, String> parameterMap)
              throws ReadNotPossibleException, SessionExpiredException;
}

Now you are forced to create a SessionHandle instance that is pre-verified, and immutable. The content of that SessionHandle is whatever it needs to be for your actual IDBClient implementaton. The IDBClient implementation will know which handles are valid, and can do other things with the handle as needed.

Because times are good, and Java7 is better, you should also ensure that your SessionHandle is AutoCloseable so you can use try-with-resources.

Your Exceptions (which you should create custom versions of) for your interface should all extend a single Exception class, for example:

public DBClientException extends Exception { ..... }
public ReadNotPossibleException extends DBClientException {....}
public SessionExpiredException extends DBClientException {...}

by doing this, you can allow the user to catch just the database exception, or more-specific database exception sub-types if needed....

The 'user experience' will look like:

IDBClient client = DatabaseClientFactory.getInstance();
try (IDBClient.SessionHandle session =
          client.createSession( 109739281L, 20L, 1000L, true) ) {

    String result = client.read(session, paramMap);

} catch (DBClientException e) {
    .....
}


Operator-based switch for a database query filter I am currently working on a method, that can search based on a value and an operator.

Currently I check which operator was supplied to the the method in a switch, and do operations accordingly to it.

This is the current code:

switch (enumOperator)
{
    case Operator.Equal:
        tmpResult = tmpResult.Where(a => a.NUMERIC_VALUE == value);
        break;
    case Operator.Less:
        tmpResult = tmpResult.Where(a => a.NUMERIC_VALUE < value);
        break;
    case Operator.LessOrEqual:
        tmpResult = tmpResult.Where(a => a.NUMERIC_VALUE <= value);
        break;
    case Operator.More:
        tmpResult = tmpResult.Where(a => a.NUMERIC_VALUE > value);
        break;
    case Operator.MoreOrEqual:
        tmpResult = tmpResult.Where(a => a.NUMERIC_VALUE >= value);
        break;
    case Operator.NotEqual:
        tmpResult = tmpResult.Where(a => a.NUMERIC_VALUE != value);
        break;
}

Is there a better/more succinct way to represent the previous switch in c#?

Additional info

The previous piece of code is used to search in a database based on the parameter(the field to be searched by), the value and operator.

So the previous code is copied for each field (a => a.X, a => a.Y ...).

As some of the fields are of different type, the "ugly" code is a bit changed and copied again.

Besides of the big switch (which by @unholysampler answer) will be nicer, the code a => a.X == value will be repeated many times(3).

Would it be also possible to save predicates where a => a.X == value, X is yet to be specified?

There are a few ways to approach this base on how much you want to minimize the code.

Define the predicates as actual functions and pass them in by name.

case Operator.Equal:
    tmpResult = tmpResult.Where(NumericEqual);
    break;

Use a temporary variable for the argument to Where()

Func<T, Boolean> predicate;
switch (enumOperator)
{
    case Operator.Equal:
        predicate = a => a.NUMERIC_VALUE == value;
        break;
    case Operator.Less:
        predicate = a => a.NUMERIC_VALUE < value;
        break;
    // ...
}
tmpResult = tmpResult.Where(predicate);

This could also be done with a function that encapsulates the switch and hiding it from the main function.

Or, if you feel like the switch statement is causing too much boilerplate, you could make a Dictionary<Operator, Func<T, Boolean>> and insert a function pointer for each operator. Then the code that uses it becomes:

tmpResult = tmpResult.Where(operatorMap[enumOperator]);

It is a trade off. The shorter code requires the use of more memory. Handling the case where enumOperator does not have match (which is not currently being done) would also have to change.

For the case where you will have lots of different properties that all need similar operations, you can have a function that creates the predicate.

Func<T, Boolean> MakePredicateEqual(Func<T, X> getField, X value)
{
    return a => getField(a) == value;
}

However, as things become more generic, more things need to be passed in as arguments and plumbing things together gets more complicated. It needs to be determined when the diminishing returns make it not worth it.


Mutually exclusive properties The question is probably quite simple, but I would like to hear what drawbacks we will have with our code.

So, we had a simple implementation and interface for it:

public interface ISettings
{
    bool IsOnline { get; set; }
}

public class Settings : ISettings
{
    public bool IsOnline { get; set; }
}

The property shows the application state and used in many conditions. The interface in our case is required for implementing settings for different platforms.

My colleague insists on implementing an additional property:

public interface ISettings
{
    bool IsOnline { get; set; }
    bool IsOffline { get; set; }
}

public class Settings : ISettings
{
    public bool IsOnline { get; set; }
    public bool IsOffline
    {
        get { return !IsOnline; }
        set { IsOnline = !value; }
    }
}

Is it a good practice to have such additional properties just for convenient code-reading?

Just a quick note: a Status (or ConnectionStatus) enum with ONLINE and OFFLINE values would be more readable here.


Sherlock and Anagrams in Javascript I am trying to solve a Hackerrank problem Sherlock and Anagrams. I think my code is valid but somehow times out for some of the test cases. Any tips on how to improve its performance? Critique unrelated to performance is welcome as well!
Here is my main method which gets a string as an input and returns an integer that represents the number of pairs of substrings of the string that are anagrams of each other:
function sherlockAndAnagrams(s) {

    let substrings = [];
    let pairs = [];

    // get all substrings
    for (let i = 0; i < s.length+1; i++) {
        for (let j = i+1; j < s.length+1; j++) {
            substrings.push(s.substring(i, j));
        }
    }

    // sort substrings so they can be compared using ===
    substrings = substrings.map(sub => [...sub].sort().join(""));

    // count them by comparing each with each skipping the ones with different length
    return substrings.reduce((counter, sub, index, arr) => {
        return counter += arr.reduce((acc, item, jndex) => {
            if (sub.length === item.length && sub === item && index !== jndex) {
                let uid = [index, jndex].sort().join("");
                if (!pairs.includes(uid)) {
                    acc++;
                    pairs.push(uid);
                }
            }
            return acc;
        }, 0);
    }, 0);
}

Here is a shortened version of the problem description as additional context:

Two strings are anagrams of each other if the letters of one string can be rearranged to form the other string. Given a string, find the number of pairs of substrings of the string that are anagrams of each other.
sherlockAndAnagrams has the following parameter: s: a string
It must return an integer that represents the number of anagrammatic pairs of substrings in s. For each query, return the number of unordered anagrammatic pairs.

comparing each with each means the quadratic time complexity, and skipping the ones with different length does not improve the bottomline.

Sort the set of (sorted) substrings, and the anagrams will form contiguous runs. The run of length \$k\$ produces \$\dfrac{k(k-1)}{2}\$ pairs.

Example:

An original array:

ab
cde
xy
dec
ba
ced

After each string has been sorted, it becomes

ab
cde
xy
cde
ab
cde

Now, after the entire array is sorted lexicographically, it becomes

ab
ab
cde
cde
cde
xy

The ab, ab and cde, cde, cde form the contiguous (that is, uninterrupted) runs, of length 2 and 3 respectively.


Finding integer lengths for a right triangle with a given perimeter I'm using Python 3 and have written this code to find the right triangle sides when given the perimeter:

def intRightTri(p):
    l = [(a,b,c) for a in range (1, p)
                 for b in range (1, p)
                 for c in range (1, p)
                 if (a**2 + b**2 == c**2) and (a + b + c == p) and (a<=b) and (b<c)]
    return l

When I run my tests, they work, but the last one seems to be taking a really long time. What would be a good way to improve my code so it executes quicker?

import unittest
tc = unittest.TestCase()
tc.assertEqual(intRightTri(100), [])
tc.assertEqual(intRightTri(180), [(18, 80, 82), (30, 72, 78), (45, 60, 75)])
tc.assertEqual(intRightTri(840),
           [(40, 399, 401),
            (56, 390, 394),
            (105, 360, 375),
            (120, 350, 370),
            (140, 336, 364),
            (168, 315, 357),
            (210, 280, 350),
            (240, 252, 348)])

Update

New quicker method combining both answers.

def integer_right_triangles(p):
    l = [(a,b,p-a-b) for a in range (1, p)
            for b in range (a, p-a)
            if (a**2 + b**2 == (p - (a+b))**2 )]
    return l

You can improve the performance with a bit of math. Rather than doing this in \$O(p^3)\$ you can do it in in \$O(p)\$.

First remove \$c\$ from the equation:

$$
a + b + c = p\\
c = p - a - b\\
a^2 + b^2 = c^2\\
a^2 + b^2 = (a + b - p)^2$$

After this you can expand the right hand side and simplify.

$$
a^2 + b^2 = (a + b - p)(a + b - p)\\
a^2 + b^2 = a^2 + b^2 + 2ab - 2ap - 2bp + p^2\\
2ab - 2ap - 2bp + p^2 = 0\\
a = \frac{2bp - p^2}{2b - 2p}\\
$$

Which in Python can be:

def _right_a(p):
    for b in range(1, p // 2):
        a = (2*b*p - p**2) / (2 * (b - p))
        if a % 1:
            continue
        a = int(a)
        yield a, b, p - a - b

def int_right_tri(p):
    return {tuple(sorted(i)) for i in _right_a(p)}