Pig Latin Translator in Java For starters, I'm a student taking AP Computer Science this year, loving it so far. Lots of fun, especially problems like this. I think I'm going to make it a regular practice to post some of my exercises and projects here for people to review, so I can get some feedback from programmers other than my teacher on different ways to do things, ways to clean up my code, and good practice.

I'm not looking for answers here. I've completely written the code and am just looking for some feedback on a few questions listed below.

The prompt is as follows:

  Write an interactive program that reads lines of input from the user
  and converts each line into Pig Latin. Terminate the program when the
  user types a blank line.

  Words beginning with consonants have the consonant moved to the end of the word and "ay" appended
  Words beginning with vowels simply have "ay" appended

Specific questions:

Did I do this in an acceptably efficient way? What other, possibly better, ways could I have accomplished this?
How could this be done without an ArrayList? Learning how to use ArrayLists is something I've done on my own time, not that we've learned in class, and I want to become more independent from them on problems like this in case my teacher doesn't want me to keep using them.
What variables do I need to keep as class variables? When I was declaring them I wasn't sure exactly how I would structure the program, or if I was going to organize it into separate methods or not.

import java.util.*;
public class PigLatin {
    static ArrayList<String> al = new ArrayList<String>();
    static Scanner sc = new Scanner(System.in);
    static String userString;
    static String latinString;
    static String temp;

    public static void main(String[] args) {
        while (true) {
            latinString = "";
            System.out.print("Enter a string to be converted into Pig Latin. To stop, enter a blank input: ");
            String userString = sc.nextLine();
            if (userString.isEmpty()) {
                break;
            }

            ArrayList<String> al = new ArrayList<String>(Arrays.asList(userString.split("\\s")));

            for(int i = 0; i <= al.size() - 1; i++) {
                temp = al.get(i);
                String tester = temp.toUpperCase();
                if (tester.charAt(0) == 'A' || tester.charAt(0) == 'E' || tester.charAt(0) == 'I' || tester.charAt(0) == 'O' ||
                        tester.charAt(0) == 'U' ) {
                    latinString = latinString + temp + "ay ";
                } else {
                    latinString = latinString + temp.substring(1) + temp.charAt(0) + "ay ";
                }

            }
            System.out.println(latinString);
        }
        sc.close();
    }
}

Scoping

All the static fields of your class can be made into local variables. This gives them a smaller scope. Always give variables the smallest possible scope.
When you do this you'll notice userString and al are already defined as a local variable.
al should be declared as a List<String>, in fact you'll see that the code works fine with the List returned by Arrays.asList().

Naming

Use meaningful names instead of abbreviations for names : al, sc, even temp don't convey anything.

Algorithm

Testing whether the first letter of a String is a vowel shouldn't require you to uppercase the entire String. You shouldn't even be making a second String (tester).
It's easier to make a String of all vowels (upper and lower case) and use indexOf() on that string with the character you want to test.

The while(true) loop is bogus too. I had to look for the exit condition. Make it explicit.

You remember to close the Scanner, but do it in a finally block, so a possible Exception doesn't bypass the closing. For extra points use a try-with-resources structure (see my refactored example).

Wait, remember I said the code would work fine with the result of Arrays.asList()? It will, but you don't even need a list to loop over an array. Just use an advanced for loop.

for (String temp : userString.split("\\s")) {
    ...
}

latinString should be a StringBuilder, as it's mostly used to build the resulting String.

Use helper methods to explain what you're doing or to extract repeated code.

Here's my refactored version :

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) { // try with resource will ensure the Scanner gets closed.
        String userString = getUserInput(scanner);
        while (!userString.isEmpty()) {
            System.out.println(buildLatinString(userString));
            userString = getUserInput(scanner);
        }
    }
}

private static String buildLatinString(String userString) {
    StringBuilder latinString = new StringBuilder();
    for (String word : userString.split("\\s")) {
        latinizeWord(latinString, word);
    }
    return latinString.toString();
}

private static void latinizeWord(StringBuilder latinString, String word) {
    if (startsWithVowel(word)) {
        latinString.append(word).append("ay ");
    } else {
        latinString.append(word.substring(1)).append(word.charAt(0)).append("ay ");
    }
}

private static String getUserInput(Scanner scanner) {
    System.out.print("Enter a string to be converted into Pig Latin. To stop, enter a blank input: ");
    return scanner.nextLine();
}

private static boolean startsWithVowel(String word) {
    return "AEIOUaeiou".indexOf(word.charAt(0)) != -1;
}


LPTHW - ex48 - Handling exceptions and unit testing This is my solution to the exercise 48 of Learn Python the hard way by Zed Shaw. Please visit the link for testing suite and requirements.

I'm worried about my the word banks I have created (COMPASS, VERBS, etc...) as this seems a duplication from the testing code.

I'm also afraid my handling might have gone overboard as all I needed it for was finding out which string items could be converted to integers.

1 COMPASS= ['north','south','east','west']$
2 VERBS = ['go', 'kill', 'eat']$
3 STOPS = ['the', 'in', 'of']$
4 NOUNS = ['bear', 'princess']$
5   $
6 import string$
7 def scan(sentence):$
8     action = []$
9     #TODO: it seems there might be a way to  combine these if statments$
10     for word in sentence.split(' '):$
11         lword = string.lower(word)$
12         try:$
13             if lword in COMPASS:$
14                 action.append(('direction', word))$
15             elif lword in VERBS:$
16                 action.append(('verb', word))-$
17             elif lword in STOPS:$
18                 action.append(('stop', word))$
19             elif lword in NOUNS:$
20                 action.append(('noun', word))$
21             elif int(lword):$
22                 action.append(('number', int(word)))$
23         except:$
24             action.append(('error', word))$
25     return action$
26 $

lword = string.lower(word)

There is no need to import the string module for this, you can use:

lword = word.lower()

As for this:

elif int(lword):

Probably better as

elif lword.isdigit():

As for your exception handling

     except:
         action.append(('error', word))

You should almost never use a bare except. You should catch a specific exception, and put a minimal amount of code in the try block. In this case you could have done something like

try:
   actions.append( ('number', int(word) )
except ValueError:
   actions.append( ('error', word) )


Choosing the desired icon when fetching results I'm creating a Q&A website, Something like SO. Now I need to print a "check" icon (to specify accepted answer) next to every answer. Now there are some conditions:

Is the current user logged?
Is the current user the author of the question?
Is there any accepted answer?

And some other cases.

Actually, I implemented that. But I did it using a lot of nested if-statement and seems to me really unoptimized. Now I want to know is there any better algorithm to write this code?

$results = $stm->fetchAll(PDO::FETCH_ASSOC);       // Fetching all answers

foreach ($results as $result){

    if ($_SESSION['id'] == $author_id){            // User is logged
        if($result['id'] == $AcceptedAnswerId){    // There is accepted answer
            $AcceptAnswerIcon = "

                                 ";
        } else {                                   // There isn't accepted answer
            $AcceptAnswerIcon = "

                                 ";
        }
    }

    elseif ($_SESSION['id'] !== $author_id) {      // User isn't logged
        if($result['id'] == $AcceptedAnswerId){    // There isn accepted answer
            $AcceptAnswerIcon = "";
        } else {                                   // There isn't accepted answer
            $AcceptAnswerIcon = null;
        }
    }

    else {
        $AcceptAnswerIcon = null;
    }

// ... I will use $AcceptAnswerIcon in here and mix it with answer structure

} // End of "foreach" block

Extract Duplication to Function

You could try to extract some of the duplication:

getCheckIconHTML($color) {
    return "";
}

Now all the check icon HTML is in one place, which also makes it easier to change.

But the HTML really doesn't only depend on the color, but it also matters if it is clickable or not, so lets add that as well:

getCheckIconHTML($color, $isClickable) {
    $checkIcon = "";
    if ($isClickable) {
        $checkIcon = "" . $checkIcon . "";
    }
    return $checkIcon;
}

Improve the ifs

Now, the foreach would look like this, which is already better:

foreach ($results as $result){

    if ($_SESSION['id'] == $author_id) {            // User is logged
        if ($result['id'] == $AcceptedAnswerId) {
            $AcceptAnswerIcon = getCheckIconHTML('green', true);
        } else {
            $AcceptAnswerIcon = getCheckIconHTML('grey', true)
        }
    } else {      // User isn't logged
        if ($result['id'] == $AcceptedAnswerId) {
            $AcceptAnswerIcon = getCheckIconHTML('green', false);
        } else {
            $AcceptAnswerIcon = null;
        }
    }
}

But now that we have the function, we can also restructure the ifs to get this:

$isUserLoggedIn = ($_SESSION['id'] == $author_id);
if ($result['id'] == $AcceptedAnswerId) { // There is accepted answer
    $AcceptAnswerIcon = getCheckIconHTML('green', $isUserLoggedIn);
} else {
    $AcceptAnswerIcon = $isUserLoggedIn ?
            getCheckIconHTML('grey', true) : null;
}

Misc

you don't need the $_SESSION['id'] !== $author_id check in the elseif. You already checked it in the if, so you know that the user is not logged in. If the last else is entered, something went wrong.
variable names should start with a lower-case letter.
your spacing is sometimes off.
I would move the code that checks if a user is logged in into a user class or something. If you just have checks like this all over the place, it will be hard to change the functionality later on.


My custom implementation of MATLAB's imdilate I have been assigned to implement the morphological dilation operation without using MATLAB's imdilate, and I came up with the following solution:

% -------------------------------------------------------------------------
% -> Morphological Operations
% -> Image Dilation
% -------------------------------------------------------------------------

% -------------------------------------------------------------------------
% - [Pre-Operation Code: Getting Everything Ready] -
% -------------------------------------------------------------------------
% Starting With A Clean (Workspace) And (Command Window)
clear;
clc;

% Creating A Matrix Consisting Of 0s And 1s Representing A Binary Image.
binaryImage = [ ...
    0 0 0 0 0 0
    0 0 1 1 0 0
    0 0 1 1 0 0
    0 0 0 0 0 0];

% Creating A Matrix Representing Our Structuring Element
structuringElement = [ ...
  0 1 0
  1 1 1
  0 1 0];

% Getting The Number Of Rows (Height) And The Number Of Columns (Width) Of The Binary Image
[imageRows, imageColumns] = size(binaryImage);

% Getting The Number Of Rows (Height) And The Number Of Columns (Width) Of The Structuring Element
[structuringRows, structuringColumns] = size(structuringElement);

% Creating An Empty Matrix That Will Be Used To Store The Final Processed Image
dilatedImage = zeros(imageRows, imageColumns);
ref = imdilate(binaryImage, structuringElement);
% -------------------------------------------------------------------------
% - [Dilation Operation] -
% -------------------------------------------------------------------------
% Going Over Each Row In The Binary Image
for i = 1:imageRows
  % Going Over Each Column In The Binary Image
  for j = 1:imageColumns
    % If The Current Pixel Is A Foreground Pixel (1)
    if (binaryImage(i, j) == 1)
      % Going Over Each Row In The Structuring Element
      for k = 1:structuringRows
        % Going Over Each Column In The Structuring Element
        for l = 1:structuringColumns
          % If The Current Pixel In The Structuring Element Is A Foreground Pixel (1)
          if (structuringElement(k, l) == 1)
            dilatedImage(i + k - 2, j + l - 2) = 1;
          end
        end
      end
    end
  end
end

subplot(1, 3, 1), imshow(binaryImage), title('Original Image');
subplot(1, 3, 2), imshow(dilatedImage), title('Dilated Image');
subplot(1, 3, 3), imshow(ref), title('MATLAB imdilate');

I am open to any suggestions! Thank you in advance.

I have a few different comments.

Firstly, for 2D arrays like in your example, you can simply do conv2(binaryImage,structuringElement,'same'), it's equivalent to imdilate (and about 4 times faster). That might not be acceptable for your case if you are supposed to write the code yourself though.

Next, and importantly, your code has a logic error. If your binary image has a white pixel along any edge, the results are incorrect or you get an error (if it's in the top row). This is due to your indexing into dilatedImage where i+k-2 can be less than 1, or can be larger than the number of rows in dilatedImage.

You have a lot of comments, which is better than none, but I think it's excessive here. It is very clear what this line of code does [imageRows, imageColumns] = size(binaryImage);, for example, even for people who don't know Matlab. Some of your comments make the lines very long as well.

A minor issue, I'd move the imdilate command somewhere else, that part of the code is where you are re-creating imdilate, I think it's nicer to separate the verification part.

Another minor issue is in using i and j as loop indices. Since i and j can be used for complex numbers in Matlab, by convention they are avoided as loop indices (although I do not usually worry about this).

I would put the code that does the work into its own function that takes binaryImage and structuringElement as inputs and returns dilatedImage.

The big thing here is you 6 nested for/if statements, which is hard to read and hard to code. Historically, such nested loops would have been very slow in Matlab, but nowadays it's not such a concern. One thing you should do is swap the order of the first two for loops. Matlab stores arrays in a column major format, so loop over the columns first, then the rows. This gives roughly a 20-25% speedup in my tests. Additionally, checking to remove the out-of-bounds error is also a speedup, since we only set pixels to one if they are inside the image, irregardless of the value of the structuring array. Check out this version of your loop:

for j = 1:imageColumns
    for i = 1:imageRows
        if binaryImage(i, j) == 1
            % Loop through the entries of the structuring element
            for l = 1:structuringColumns
                for k = 1:structuringRows
                    % make sure the new pixel is inside the image
                    if  i+k-2>0 && j+l-2>0 && i+k-2<=imageRows && j+l-2<=imageColumns
                        % make a white pixel if necessary
                        if structuringElement(k, l)==1
                            dilatedImage(i + k - 2, j + l - 2) = 1;
                        end
                    end
                end
            end
        end
    end
end

Of course it would be nice to avoid writing all these loops, and that can be done. This next method is only slightly faster, but to me is cleaner. The idea is to first find the white pixels, and just loop over them, for each one working out which surrounding pixels should be changed.

[rowIdx,colIdx] = find(binaryImage); % the indices of white pixels in the image
[maskRowIdx,maskColIdx] = find(structuringElement); % indices of structuring elements

% now get indices of structuring elements relative to the centre of the array
maskRowIdx = maskRowIdx - floor(structuringRows/2) - 1;
maskColIdx = maskColIdx - floor(structuringColumns/2) - 1;

dilatedImage = zeros(imageRows, imageColumns);

% loop over each white pixel in the image
for i = 1:numel(rowIdx)
    % these are just the indices of the pixel
    rI = rowIdx(i);
    cI = colIdx(i);

    % now loop over each non-zero element of the structuring array
    for j = 1:numel(maskRowIdx)
        % the position of the pixel to change is (r,c)
        r = rI+maskRowIdx(j);
        c = cI+maskColIdx(j);

        % if the pixel is inside the image, we make it a 1
        if r>0 && c>0 && r<=imageRows && c<=imageColumns
            dilatedImage(r,c) = 1;
        end
    end
end


Generic numerical systems conversion This program is a start point to a more comprehensive program that deals with different numerical systems from binary until 35 base. That includes the traditional decimal, octal and hex.

It is until 35 only, because no more digits other than the number 0-9 and the letters A-Z to represent digits for higher base systems.

The program needs a lot of validation code and other stuff. However, I would like to ask if the conversion from/to the decimal is good or you can show me a better approach.

My main focus is the algorithms for the conversion functions and the helper functions.

Notes:

I do not want to use the built-in pow() function; I wrote my own version.
I do not want to use ASCII in character conversions.

#include <iostream>
#include <string>

using namespace std;

string digit = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";

int power(int base, int exp)
{
    int result = 1;

    for (int i = 0; i < exp; ++i)
        result = result * base;

    return result;
}

int val(char ch)
{
    ch = toupper(ch);

    for (int i = 0; i < 37; ++i)
        if (digit[i] == ch)
            return i;

    return -1;
}

char chr(int d)
{
    return digit[d];
}

string int_to_string(int n)
{
    string result = "";
    int divisor = 10;
    int num = n;
    int r;
    int length = 0;

    while (num > 0)
    {
        num = num / 10;
        ++length;
    }

    num = n;
    divisor = power(10, length) / 10;

    while (num != 0)
    {
        r = num / divisor;
        num = num % divisor;
        result += chr(r);
        divisor /= 10;
    }

    return result;
}

string to_decimal(string str, int base = 10)
{
    if (base == 10) return str;
    if (base < 2 || base > 37) return "Error: base out of range";
    int length = str.length();
    if (length < 1) return "Error: No Number";
    int result = 0;

    for (int i = length - 1; i >= 0; --i)
        result = result + power(base, length - i - 1) * val(str[i]);

    return int_to_string(result);
}

string from_decimal(string str, int base = 10)
{

    if (base == 10) return str;
    if (base < 2 || base > 37) return "Error: base out of range";
    int length = str.length();
    if (length < 1) return "Error: No Number";

    string result = "";
    int divisor = base;
    int num = 0;
    int r;

    for (int i = 0; i < length; ++i)
        num = num + power(10, length - i - 1) * val(str[i]);

    while (num != 0)
    {
        r = num % divisor;
        num = num / divisor;
        result = chr(r) + result;
    }

    return result;
}

bool is_valid(string str, int frombase)
{
    for (char ch : str)
        if (val(ch) >= frombase)
            return false;
    return true;
}

string baseconvert(string str, int frombase,int tobase)
{
    if(is_valid(str, frombase))
        return from_decimal(to_decimal(str,frombase), tobase);
    else
        return "Error: Improper Number used!\n";
}

int main()
{
    cout << baseconvert("FZ", 16,8) << "\n\n\n";
    cout << baseconvert("FE", 16,2) << "\n\n\n";
}

Instead of trying to create your own versions, you could try doing it without.

Not wanting to use ASCII conversions makes sense since that would be inefficient.  using a string to get the char value from a specific integer value, works well.  However, to go the other way a map would work better, and ignores how the native mapping works.

map<char,int> values =
{
    {'0',0},
    {'1',1},
    {'2',2},
    {'3',3},
    {'4',4},
    {'5',5},
    {'6',6},
    {'7',7},
    {'8',8},
    {'9',9},
    {'A',10},
    {'B',11},
    {'C',12},
    {'D',13},
    {'E',14},
    {'F',15},
    {'G',16},
    {'H',17},
    {'I',18},
    {'J',19},
    {'K',20},
    {'L',21},
    {'M',22},
    {'N',23},
    {'O',24},
    {'P',25},
    {'Q',26},
    {'R',27},
    {'S',28},
    {'T',29},
    {'U',30},
    {'V',31},
    {'W',32},
    {'X',33},
    {'Y',34},
    {'Z',35}
};

Using a power function  reiterates over the same loop over and over again each time you call it.  It would make more sense to start at the end of the string and increase a multiplier in each iteration of the existing loop.  Also when working with base 10 it is alot simpler to keep the values as integers:

int ToBase10(string value, int base)
{
    int decValue = 0;
    int baseMult = 1;
    for (int i = value.size()-1; i > -1; i--)
    {
        decValue += values[value[i]] * baseMult;
        baseMult *= base;
    }
    return decValue;
}

Since decimal has other meanings, it would be clearer to say base10 instead.

Lastly wrapping your functions up in a static class allows you to expose only what needs to exposed.  Also this keeps the validation in only one place:

#include <map>
using std::map;
using std::string;
class BaseConverter
{
    static map<char, int> values;
    static bool GoodValue(string& value, int base);
    static string FromBase10(int value, int base);
    static int ToBase10(string value, int base);
public:
    static string Converter(string value, int fromBase, int toBase);
    static string Converter(int decValue, int base);
};

map<char,int> BaseConverter::values =
{
    {'0',0},
    {'1',1},
    {'2',2},
    {'3',3},
    {'4',4},
    {'5',5},
    {'6',6},
    {'7',7},
    {'8',8},
    {'9',9},
    {'A',10},
    {'B',11},
    {'C',12},
    {'D',13},
    {'E',14},
    {'F',15},
    {'G',16},
    {'H',17},
    {'I',18},
    {'J',19},
    {'K',20},
    {'L',21},
    {'M',22},
    {'N',23},
    {'O',24},
    {'P',25},
    {'Q',26},
    {'R',27},
    {'S',28},
    {'T',29},
    {'U',30},
    {'V',31},
    {'W',32},
    {'X',33},
    {'Y',34},
    {'Z',35}
};

string BaseConverter::Converter(string value, int fromBase, int toBase)
{
    if(!GoodValue(value,fromBase))
    {
        return "Improper Value";
    }
    if (fromBase == toBase)
    {
        return value;
    }
    return FromBase10(ToBase10(value,fromBase),toBase);
}
string BaseConverter::Converter(int decValue, int base)
{
    return FromBase10(decValue,base);
}

bool BaseConverter::GoodValue(string& value, int base)
{
    for(int i = 0; i < value.length();i++)
    {
        value[i] = toupper(value[i]);
        if (values[value[i]]>= base)
        {
            return false;
        }
    }
    return true;
}

string BaseConverter::FromBase10(int value, int base)
{
    const string chars = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    string baseValue = "";
    while (value > 0)
    {
        baseValue.insert(baseValue.begin(), chars[value % base]);
        value /= base;
    }
    return baseValue;
}

int BaseConverter::ToBase10(string value, int base)
{
    int decValue = 0;
    int baseMult = 1;
    for (int i = value.size()-1; i > -1; i--)
    {
        decValue += values[value[i]] * baseMult;
        baseMult *= base;
    }
    return decValue;
}

Notice the simplicity of only having to call one wrapper function(string test = BaseConverter::Converter("291", 10, 16);)


C++ Generic Callback class with removable listeners by unique id I'm quite new to the STL.
Does this make sense? Is there a better way of removing the listeners instead of using shared_ptr while keeping the code short and simple? Is there something in the STL to replace this?
Class:
template<class ... Arguments>
class Callback
{
    using Function = std::function<void(Arguments ...)>;
    using FunctionPtr = std::shared_ptr<Function>;
    using Functions = std::vector<FunctionPtr>;

public:
    FunctionPtr addListener(Function f) {
        FunctionPtr fp = std::make_shared<Function>(f);
        _functions.push_back(fp);
        return fp;
    }

    void removeListener(FunctionPtr fp){
        _functions.erase(std::find(_functions.begin(), _functions.end(), fp));
    }

    void operator()(Arguments ... args) const{
        for (auto & f : _functions)
            f.get()->operator()( args ... );
    }

private:
    Functions _functions;
};

Use case:
int main()
{
    Callback<int> callback;

    callback.addListener([](int i){
        std::cout << "Listener 1 -> Input is " << i <<  std::endl;
    });

    auto id = callback.addListener([](int i){
        std::cout << "Listener 2 -> Input is " << i <<  std::endl;
    });

    callback.addListener([](int i){
        std::cout << "Listener 3 -> Input is " << i <<  std::endl;
    });

    callback.removeListener(id);

    callback(71);
}

Output is:
Listener 1 -> Input is 71
Listener 3 -> Input is 71

I think this makes sense in terms of STL use.  Storing a std::shared_ptr<std::function> is not usually necessary since std::function may own heap allocations internally, but since you return the shared_ptr from addListener it could be passed around.  If you never use the returned shared_ptr, you could just store a std::vector<std::function> and return a handle for removal (maybe the handle could contain the index of the function in the vector?).
I can't say whether a vector of functions makes sense for your use case.  If you can avoid callbacks and call functions directly, that's definitely preferable -- makes optimization much easier for the compiler.

Other stuff:
class Callback
{
    using Function = std::function<void(Arguments ...)>;

These typedefs are nice, but could be better.  They should be public since they are used in the public API.  And perhaps the names could be clearer (e.g. VecPtrFunc instead of Functions)?
_functions.erase(std::find(_functions.begin(), _functions.end(), fp));

This seems OK, but if you call this a lot or if functions is big, this will be slow. You might prefer to use a data structure with sub linear lookup/delete like std::map or std::unordered_map.  Or you could use the index-handle idea above and remove from a vector more efficiently.
void operator()(Arguments ... args) const

If you were just calling one callee, this would be Arguments&& and you'd std::forward the args.  Since you use the args... in a loop, I guess this is fine.  Maybe Arguments const& is better in case folks pass in big types and don't want to copy?


Basic login with router It's not finished yet, but I want to know if the structure and the classes are ok and what can I change. Feel free to say anything.

To be more specific, I want to know what you think about the login function in the Login_Controller class:

public function login() {
            if (!empty($_POST['username']) && !empty($_POST['password'])) {
                    $user = new Users_Model;

                    $username = $_POST['username'];
                    $password = $_POST['password'];

                    if ($iduser = $user->is_registered($username, $password)) {
                            $_SESSION['user'] = $username;
                            $_SESSION['iduser'] = $iduser;
                            if ($_POST['keep']) {
                                    $user->save_cookie($username);
                            }
                            header('Location: ' . SITE_ROOT);
                            exit();
                    } else {
                            $error = 'El usuario o el password no son correctos.';
                            $this->main($error);
                    }
            } else {
                    $error = 'No ingreso el usuario o el password.';
                    $this->main($error);
            }
    }

And the router as well:

class Router {
    static function init($request)
    {
            $parsed = explode('/', $request);
            if ($parsed[0] == 'admin') {
                    $page = array_shift($parsed).ucfirst(array_shift($parsed));
            } else {
                    $page = array_shift($parsed);
            }
            if (empty($page)) {
                    $page = 'index';
            }
            $action = array_shift($parsed);
            if (empty($action)) {
                    $action = 'main';
            }
            if (empty($parsed)) {
                    $arguments = 0;
            } else {
                    $arguments = $parsed;
            }
            $target = SERVER_ROOT . '/controllers/' . $page . '.php';
            if (file_exists($target)) {
                    include_once $target;
                    $class = ucfirst($page) . '_Controller';
                    if (class_exists($class)) {
                            $controller = new $class;
                    } else {
                            die('la clase no existe');
                    }
            } else {
                    die('la pagina no existe');
            }
            $controller->$action($arguments);
    }
}

They are ok, but I would still do some things differently.

I would instead throw exceptions.
Instead of if else within your action, I would use a validator with rules.
Instead of using $_POST/$_GET directly, I would sanitize them and put it inside a Request class.
Error messages belongs to the view, so I would set $view->errors[]='message' or something like that.
Your routing class is not flexible. As a good example, you could take a look at the Silex framework.
Login_Controller should not exist. There might be Account_Controller with login_action or Authenticate_Controller with login_action.

Usually a Controller is a group of actions with to execute specific stuffs. For example, Profile_Controller has view/edit/delete action. Within the controller you check the user roles, if someone can view/edit/delete the profile. In other frameworks, they use "Access Control List (ACL)" for this purpose.

Your linked tutorial is just for the basic understanding purpose, so please don't use it. Use a readily existing framework, since you wish to build the app and not the framework itself. You might end with modifying and changing your basic structures and implementing new functions instead of developing the features of your web application.


Data validation for preventing blank entries I want to create a validation wherein blank entries will not be accepted. So if I call this code:

entry[i].setName(JOptionPane.showInputDialog("Enter Name: "));
if the entry is blank, it will not be accepted and an error will prompt:

it cannot accept blank entries. Of course it could easily remedied with this code:

String name = JOptionPane.showInputDialog("Enter Name: ");
 while (name.equals("")){
       JOptionPane.showMessageDialog(null, "Cannot accept blank entries!");
       name = JOptionPane.showInputDialog("Enter Name: ");
 }

but if I want to validate 100 fields that I don't want to have a blank entry, then my code will be messy and long.

How could I do it better? I've read about using getters and setters or the try-catch methods to do the validation but I don't know if this kind of validation is applicable. And I don't know how I can do it. And if it is applicable, would I be violating the Model-View-Controller concept if I included a JOption message dialog box on my getter and setter methods? What code does programmers usually use in doing blank entries validation?

Extract the input code into a method:

String getNonBlankInput(String prompt) {
   String input = JOptionPane.showInputDialog(prompt);

   while (input.equals("")) {
      JOptionPane.showMessageDialog(null, "Cannot accept blank entries!");
      input = JOptionPane.showInputDialog(prompt);
   }

   return input;
}

Usage:

String name = getNonBlankInput("Enter name: ");


Searching has_and_belongs_to_many relations for article categories I have Article and Category, with a has_and_belongs_to_many relation between the two.

I want to use Article.in_categories(category_ids) to get any Article that is in any of the category_ids passed, and I want to handle the possibility of category_ids being nil in a way that all Article items are returned. The category_ids value is coming from a form, and I want the user to be able to ignore the Category selection part of the form if they do not want to limit their selection by that criteria.

I currently have this in place on the Article model:

def self.in_categories(category_ids = nil)
  category_ids.blank? ? all : includes(:categories).where(categories: { id: category_ids })
end

If I do not have the check for category_ids being nil, then it will only return the Article items that are not in any Category.

I do not want to manually set category_ids = [1,2,3] (for example) in the method call, because a Category can be added/removed.

Is there a better way of handling this than the way I already do?

Generally you'll want to make sure this always gets passed an array so I'd choose to set the argument to an empty array rather than nil. Since in the successful case the method receives an array, you should treat it as such. If you ever expect to get a nil passed in (like you might from params), you can also catch if it is blank or empty to handle all cases safely.

If you get rid of the ternary, you can simply put a guard as the first line and that clarifies the method a lot more. You'll see the use of guards throughout the Rails source itself.

def self.in_categories(category_ids=[])
  return all if category_ids.blank? || category_ids.empty?
  includes(:categories).where(categories: { id: category_ids })
end

Alternatively you could always cast the parameter to an array and only handle that case.

def self.in_categories(category_ids=nil)
  category_ids = category_ids.to_a
  return all if category_ids.empty?
  includes(:categories).where(categories: { id: category_ids })
end

I've run into this "undocumented feature" on several occasions with Rails and it trips me up every time. Hopefully someone has a better solution so that Rails, but it is also good to just be explicit on the results if you can't consolidate.

Looks like there are quite a few discussions about this https://github.com/rails/arel/pull/245


Calculator with multiple inputs to add up Can someone look over my code and clean it up?
I am trying to make a calculator with multiple inputs to add up. This is a snippet of the code that just needs to be cleaned up and shortened and I am relatively new to python so if anyone wants to clean this up thank you.
#functions
def x():
    #needed for the the loop later
    x.x = int(input("Type how many numbers you would like to add up: "))
    x.xCON = int(input("Please confirm by typing the same number: "))

#welcome to the code
print("Would you like to add up lots of numbers?")
print("If so you came to the right place")

#neaten up
print("________________________________________________")

#run function
x()

#cheack if the numbers are the same if not they have to retype them
if x.x == x.xCON:
    #neaten up
    print("____________________________________")
    #setting veriables for the loops and the list of the numbers
    i = 0
    nums = []
    #while i is not = to the input number(x.x) do the loop
    while i != x.x:
        #asks for the numbers to be added up
        INTnum = float(input("Write the numbers to be added up: "))
        #adds the numbers to the list
        nums.append(INTnum)
        #updates the veriable cheacking how many times the code has been ran
        i+=1
    #creating new veriables for the next loop
    index = 0
    ans = 0
    #while indxex is not = to the amount to numbers in the list run the code
    while index != len(nums):
        #veriable nums is now set as the lists index of the veriable index
        num = nums[index]
        #the ans veriable is eqal to itself + the number that was recorded from the idex
        ans = ans + num
        #moves the number along one everytime the code is run
        index+=1
    #gives the rueult of the sum
    print("\nThe sum of the numbers is", ans)
    #neaten up
    print("____________________________________________________")

#what happens if the numbers are not the same sending the bacn to the x() function
else:
    print("The numbers were not the same.")
    x()

#end of the code
print("\nThank you for using the calculator")

Your x function has a few notable things:

x is a very poor name. It doesn't give any hints as to what the function is supposed to do. The name of your function should let the reader of your code know what its purpose is. Something like get_user_input would be much better.

You're associating data with the function object itself; essentially creating static variables. This is not a good idea though. This means that this function can only be called once before you overwrite the previous entered data. In your case here, that may not be a big deal, but it is not a good habit to get into. This will significantly complicate code later when you have functions that you're calling in multiple places in your code. Return the results instead.

You're having the user enter a confirmation, but then you're checking that they match outside of the function. Just check inside the function and loop until they match.

After fixing up those points, I have:
def get_user_input():
    while True:
        n_numbers = int(input("Type how many numbers you would like to add up: "))
        confirmation = int(input("Please confirm by typing the same number: "))

        if n_numbers == confirmation:
            return n_numbers
        else:
            print("The numbers were not the same.")

They you can get rid of the else case at the bottom of your code. You'd then use this function like:
n_numbers = get_user_input()

And use n_numbers instead of x.x.

You don't do any error handling anywhere. I find it odd that in your code, you require confirmation to ensure the user didn't typo the number they entered, but you aren't protecting against them typing in nonsensical input (like hello). If the user types a non-number, you'll get a ValueError that will crash your program. You may want to consider wrapping calls to int and float in a try/except to ensure that your program won't crash.

This loop:
nums = []
while i != n_numbers:
    INTnum = float(input("Write the numbers to be added up: "))
    nums.append(INTnum)
    i+=1

Can be cleaned up by using a range:
nums = []
for _ in range(n_numbers):
    INTnum = float(input("Write the numbers to be added up: "))
    nums.append(INTnum)

Now you don't need to manually handle i. This can be further cleaned up by using a list comprehension:
nums = [float(input("Write the numbers to be added up: "))
        for _ in range(n_numbers)]

Whenever you find yourself manually handling loop indices like you were with i, there's likely a better way that handles the looping for you.
The same can be said for this loop:
while index != len(nums):
    num = nums[index]
    ans = ans + num
    index+=1

Note how you never need index except to access nums. Just loop over nums directly:
for num in nums:
    ans += num  # The same as "ans = ans + num"

If you need to loop over a list, just loop over the list directly.
Really though, that's just sum:
ans = sum(nums)

Your comments aren't a good use of commenting. A lot of them are simply restating what the code itself says:
#the ans veriable is eqal to itself + the number that was recorded from the idex
ans = ans + num

. . .

#creating new veriables for the next loop
index = 0

If something is already self-explanatory, don't simply restate that in a comment. Your code reads like you're under the impression that every line of code requires a comment, but this isn't true. Ideally, your code itself should be all the explanation the reader needs, but if more explanation is required, note why the code is unreadable or why it's behaving as it is. When half your code is comments that just restate the code, it detracts from the important part: the code.


Python program to rank based on the frequency of names that appears in text files I've written a python program to rank the names that appear in the file(s) based on their frequency. In other words, there are multiple files and want to rank the frequency of the names that appears the most first, then the least at the end. Each name can appear only once in each file. In other words, the same name cannot be repeated in the same file.

Following is the program, that I've written, and wanted to hear your thoughts, on the style/efficiency and tips to improve the same

from pandas import DataFrame
filenames = [ 'Q1.txt', 'Q2.txt' , 'Q3.txt' , 'Q4.txt']
pathToDataFiles = "C:\\DataFiles\\"
outputHTMLFile =pathToDataFiles + "list.html"
df = DataFrame( columns = ['Name', 'Q1', 'Q2', 'Q3', 'Q4' ])

currentQtr = 1
for filename in filenames:
    entireFilename = pathToDataFiles  + filename
    # Get all the names from the file
    allNames = [line.rstrip('\n') for line in open(entireFilename)]
    # For each name from the latest .txt file
    for eachname in allNames:
        # If the name is in the dataframe then replace the current quarter with 'Y'
        if ( df['Name'].str.contains(eachname).any() ):
           rowNumber = df.loc[df['Name'] == eachname].index[0]
           df.iloc[[rowNumber],[currentQtr]] = 'Y'
        else:
            # It is a new name, thus, replace the current Qtr with 'Y'. Rest with be Nan
            emptyDataFrame =DataFrame( columns = ['Name', 'Q1', 'Q2', 'Q3', 'Q4' ])
            emptyDataFrame = emptyDataFrame.append([{'Name':eachname}])
            rowNumber = 0
            emptyDataFrame.iloc[[rowNumber], [currentQtr]] = 'Y'
            df = df.append(emptyDataFrame)
            df.index = range(len(df))

    currentQtr = currentQtr + 1
# Create a new column with 'Total' occurence of names
df['Total'] = currentQtr - df.isnull().sum(axis=1) - 1
# Sort by total
df = df.sort_values(by=['Total'], ascending=False)
df.index = range(len(df))
# Fill all Nan with 'X'
df = df.fillna('X')
df.to_html(outputHTMLFile)

I suggest using os.path.join instead of + to handle dangling slashes, when concatenating file names.

This variables:

filenames = [ 'Q1.txt', 'Q2.txt' , 'Q3.txt' , 'Q4.txt']
pathToDataFiles = "C:\\DataFiles\\"

could be better handled by using command line input like in argparse. And for filename you could even use os.listdir to read all files from a directory. Both would give you more flexibility in using this script.

Also, the variable eachname in for eachname in allNames: should be renamed in something like name, to prevent lines like emptyDataFrame = emptyDataFrame.append([{'Name':eachname}]) from looking weird.

There are a few incoherent whitespaces, too, and I think Nan is meant to be NaN (in comments).

Apart from that, it looks like a good start to me. I hope my answer helps and have fun coding!


Hashing user passwords I've written a Java class that will provide the utility of, in essence, hashing user passwords for an Android app that integrates Couchbase mobile into it, and then, checking whether or not the hashing of the password string entered by the user matches the hash stored in the DB to grant access.

Does there appear to be any major issues with this code to anyone? I know crypto is a very tricky thing and mistakes can be easily made.

package fitfast.security;

import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.security.spec.InvalidKeySpecException;
import java.security.spec.KeySpec;
import java.util.Arrays;

import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.PBEKeySpec;

public class Authenticator {

    public static byte[] generateHash(String password, byte[] salt) throws NoSuchAlgorithmException, InvalidKeySpecException {
        String algorithm = "PBKDF2WithHmacSHA512";
        int length = 512;
        int iterations = 60000;
        KeySpec sp = new PBEKeySpec(password.toCharArray(), salt, iterations, length);
        SecretKeyFactory kf = SecretKeyFactory.getInstance(algorithm);
        return kf.generateSecret(sp).getEncoded();

    }

    public static byte[] generateSalt() throws NoSuchAlgorithmException {
        SecureRandom sr = SecureRandom.getInstance("SHA1PRNG");
        byte[] salt = new byte[8];
        sr.nextBytes(salt);
        return salt;
    }

    public static boolean check(byte[] hash_Input, String hash_User) {
        return Arrays.equals(hash_Input, hash_User.getBytes());
    }

}

By everything I know the code looks OK from security POV.

Parameter naming convention

Your check function takes hash_User and hash_Input as parameters. This is a pretty strange naming convention. Standard Java convention would be hashUser and hashInput (which you already use in method names). hash_user and hash_input (note the lack of capital letters) would also be pretty OK.

Constants

As chillworld stated in his answer I'd extract algorithm, length and iterations as nicely named constants with some nice JavaDoc. It'll be easier to understand the code later (not an issue at this point), and easier to reuse it it next methods. While you're at it you could also declare the length of generated salt.

Encoding

Your generateHash function takes String and uses getBytes on it. Note that this is influenced by the platform's default encoding. If you later move to another platform, your hashes suddenly won't match anymore. You could force encoding by using one of the other variants of getBytes.

Verifying the password

Your check method takes byte[] hash_Input, String hash_User. Now, I'm having trouble guessing which one should be the hash stored in the DB and which one the new one user is trying to log in with. Also, how do you plan to get the one that's represented as String? Considering that the generateHash function returns byte[] you'd have to convert it to String and then back to byte array, but that's not reversible - if you use e.g. UTF-8 more than one byte sequence maps to the same String. And do you ever plan to use it really in that way? Why not having a function boolean check(byte[] hash, String password, byte[] salt) instead?

Plan for the future

This one is less clear. Suppose at some point later you want to change your algorithm/parameters (say 60000 iterations is no longer enough). Suddenly all your old hashes are no longer valid. If it's a new installation it's no problem, but what if you update installation which already has 10000 users? You can try to decode using both ways and log in if one succeeds. This increases the CPU cost of login, allows for possible collisions (unlikely). And what if you then change the algorithm second time? You could somehow encode the algorithm and parameters in the hash. This removes the possibility of collisions and later allows you to cheaply select which one of the (costly) password hashing functions to choose. And it scales well with subsequent changes. You could then update the stored hash if a user with old hash logs in. And since distinguishing users with old hash methods is easy, after some time you could ping the ones that didn't log on. The bad part: After some time one could forcefully hack your DB and replace with the weaker hash. You'll have to think carefully whether you want this one.

Plan for tests

Another not so clear one. In the current way whoever uses this class has to use this exact specification. Normally this is exactly what you want, but in case you'd like to unit-test anything that uses it the test will become unnecessary long (PBE algorithms are designed exactly to be slow, and will slow down your test in this case). You can work around this by declaring an interface with two implementations - this one, and the mock one used in tests (say generateHash returns simply password and salt concatenated and generateSalt returns a constant array). Good part: Your tests no longer need 1 second each to run. Bad part: You need to somehow get that instance in each and every class that needs it. Could be as easy as using Sprint and @Autowire on the correct field. Could be as hard as having every object own a reference to this object only to pass on to newly generated objects, while a handful of those deep in the creation chain really uses that reference. Second bad part: Somehow could inject your objects with another, weaker implementation. And this could really be a problem (compare with previous point) when you use e.g. applet or a client-side application to generate the hashes. You'd also need to consider the good and bad consequences here.


Basic "bee game" labeled as messy Could you tell me how to make it quality code?

<?php
class Game {
    public function __construct() {
        session_start();
    }
    public function play(){
        return '<input type="button" value="Play" onclick="window.location = \'?play=1\'">';
    }
    public function start($msg=null) {
        unset($_SESSION['bees']);
        $bee = new Bee();
        $bee->setType('queen');
        $this->addBee($bee,1);
        $bee->setType('worker');
        $this->addBee($bee,5);
        $bee->setType('drone');
        $this->addBee($bee,8);
        if($msg)
            return $msg;
        else {
            var_dump($_SESSION['bees']); // debug
            return 'Game started <input type="button" value="Hit" onclick="window.location = \'?hit=1\'">';
        }
    }
    public function hit() {
        $msg = '';
        if($_SESSION['bees'])
            $rd = array_rand($_SESSION['bees'],1);
        else
            $this->start('Game will start again');
        $_SESSION['bees'][$rd]['lifespan'] = $_SESSION['bees'][$rd]['lifespan'] - $_SESSION['bees'][$rd]['hit'];
        if($_SESSION['bees'][$rd]['lifespan']<1) {
            $_SESSION['bees'][$rd]['bees'] -=1;
            $_SESSION['bees'][$rd]['lifespan'] = $_SESSION['bees'][$rd]['life'];
            switch($rd)
            {
                case 'queen': $msg .= $this->start('Game start again, the Queen is dead.'); break;
                default: $msg .= 'One \'' . $rd . '\' is gone';
            }
        }
        if($_SESSION['bees'][$rd]['bees']<1){
            unset($_SESSION['bees'][$rd]);
            $msg = 'The \'' . $rd . '\' team is gone';
        }
        var_dump($_SESSION['bees']); // debug
        return '<input type="button" value="Hit" onclick="window.location = \'?hit=1\'"> ' . $msg;
    }
    public function addBee(Bee $newbee, $number = 1) {
        $tipo = $newbee->getType();
        $bee = $newbee->get();
        $_SESSION['bees'][$tipo] = $bee;
        $_SESSION['bees'][$tipo]['bees'] = $number;
        $_SESSION['bees'][$tipo]['life'] = $newbee->getLifespan($tipo);
    }
}

class Bee
{
protected $_type = null;
protected $_types = array(
    'queen' => array(
    'hit' => 8,
    'lifespan' => 100
    ),
    'worker' => array(
    'hit'   =>  10,
    'lifespan' => 75
    ),
    'drone' => array(
    hit'    =>  12,
    'lifespan' => 50
    )
    );
    public function setHit(){}
    public function setLifespan(){}
    public function addNewType(){}
    protected function _set_hit(){}
    protected function _set_lifespan(){}
    protected function _add_new_type(){}
    public function getLifespan($type){
        if(array_key_exists($type,$this->_types))
            return $this->_types[$this->_type]['lifespan'];
        else
            throw new Exception('The Bee need to be a Queen, Worker or Drone');
    }
    public function getTypes() {
        return $this->_types;
    }
    public function getType() {
        return $this->_type;
    }
    public function setType($type) {
        if(array_key_exists($type,$this->_types))
            $this->_type = $type;
        else
            throw new Exception('The Bee need to be a Queen, Worker or Drone');
    }
    public function get() {
        return $this->_types[$this->_type];
    }
}
?>
<html>
<head>
<title>play Bee</title>
</head>
<body>
    <?php
        $game = new Game();
        if(isset($_GET['play']) && $_GET['play']) {
            echo $game->start();
        } else if(isset($_GET['hit']) && $_GET['hit']) {
            echo $game->hit();
        } else {
            echo $game->play();
        }
    ?>
</body>
</html>

One big issue with this code is that you've got a tight coupling between the business logic and the presentation.  For example, the method play() really jumps out:

public function play(){

    return '<input type="button" value="Play" onclick="window.location = \'?play=1\'">';

}

From its name I'd think that this method plays something (a game? A piece of music? A role in a play? Another issue is clear, unambiguous naming of classes, methods and variables).  I wouldn't expect it to output the HTML markup for a button with a JavaScript event attached to it.

This might not seem like a big deal, and in this particular case it probably isn't.  But ask yourself this, how much work would you need to do if whoever set you this project decided half way though that it should run through the command line instead of a web browser?

Ideally, your PHP classes should contain no presentation logic at all.  They should return data structures (arrays, scalar types, simple data transfer objects, etc) to the outside world, and then a separate process should take that information and render it to the user in a suitable presentation.  The classes that do the actual work (the business objects) should implement all the logic and rules needed to accomplish a task without any concern as to how the results of that task are presented.

This concept is known as separation of concerns.  A concern is a particular part of the software package you're developing which shouldn't be tightly bound (coupled in the lingo) to other parts of your package (sorry I can't really word it better than that).  Implementing the rules and maintaining the state of the game is one concern, getting user input and displaying output to the user is a different concern.  While there obviously has to be some coupling you should try to minimize the degree that one is bound to the other.  Additionally, it's generally considered preferable for the part of the program responsible for presentation (which will typically be unique to that program) to know something about the business objects than it is for the business objects (which may get used across a wide range of applications) to know something about the presentation.

A related issue is reliance on $_SESSION inside your class.  Again this has to do with separation of concerns.  The $_SESSION array and its associated methods (session_start (), etc) are a data storage mechanism.  Data storage is a separate concern from the business logic, so again you should try to separate the code that handles storing the game state out from the code that implements the business logic.  In the case of $_SESSION there's the additional problems of it being tightly coupled to HTTP and that the session_start () function can't be called once headers have been sent to the client.  Again, ask yourself how hard would it be to adapt your code to work in a command line or to use a database instead of the PHP session to store state?


Two approaches to Logging when Unit Testing with Python Python allows logging to be used in several ways.  Can you review the relative merits of these two approaches to Logging when Unit Testing with Python?  Is one approach appropriate to specific circumstances?  What are those circumstances and how might each approach be improved?

Directly addressing the logger.

#!/usr/bin/env python
import logging
import unittest

class LoggingTest(unittest.TestCase):

    logging.basicConfig(level=logging.INFO)

    def test_logger(self):
        logging.info("logging.info test_logger")
        pass

Getting a logger by name

#!/usr/bin/env python
import logging
import unittest

class GetLoggerTest(unittest.TestCase):
    logger = logging.getLogger(__name__)
    logger.setLevel(logging.INFO)

    def test_logger(self):
        self.logger.info("self.logger.info test_logger")
        pass

According to the logging docs:

  The logger name hierarchy is analogous to the Python package hierarchy, and identical to it if you organise your loggers on a per-module basis using the recommended construction logging.getLogger(__name__). That’s because in a module, __name__ is the module’s name in the Python package namespace.

Based on my experience, I would say that you should always use logging.getLogger(__name__) for bigger projects, as it'll make logging modularization easier.

As your question is broad, I recommend you to read the Logging Cookbook, in this link, as it holds many important use cases in which you could optimize the way you use logging.

It's also possible to mix both cases in order to add a timestamp, in example, which is an improved way of using this module:

#!/usr/bin/env python
import logging
import unittest

class GetLoggerTest(unittest.TestCase):
    logger = logging.getLogger(__name__)
    logging.basicConfig(format = '%(asctime)s %(module)s %(levelname)s: %(message)s',
                    datefmt = '%m/%d/%Y %I:%M:%S %p', level = logging.INFO)

    def test_logger(self):
        self.logger.info("self.logger.info test_logger")
        pass


Checking whether a site is up or down I have this PHP curl function to check if a site is online or not:

function check($proc,$url,$keyword,$method,$timeout,$maxredirs) {
    // Format Link
    $link = $proc."://".$url;
    // Agent
    $user_agent='Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.130 Safari/537.36';

    $options = array(
        CURLOPT_CUSTOMREQUEST  => $method,     //set request type post or get
        CURLOPT_POST           =>false,        //set to GET
        CURLOPT_USERAGENT      => $user_agent, //set user agent
        CURLOPT_RETURNTRANSFER => true,     // return web page
        CURLOPT_HEADER         => false,    // don't return headers
        CURLOPT_FOLLOWLOCATION => true,     // follow redirects
        CURLOPT_ENCODING       => "",       // handle all encodings
        CURLOPT_AUTOREFERER    => true,     // set referer on redirect
        CURLOPT_CONNECTTIMEOUT => $timeout,      // timeout on connect
        CURLOPT_TIMEOUT        => $timeout,      // timeout on response
        CURLOPT_MAXREDIRS      => $maxredirs,       // stop after 10 redirects
        CURLOPT_REFERER        => "http://example.com",
        CURLOPT_SSL_VERIFYPEER => false,
    );
    $ch      = curl_init( $link );
    curl_setopt_array( $ch, $options );
    $content = curl_exec( $ch );
    $err     = curl_errno( $ch );
    $errmsg  = curl_error( $ch );
    $header  = curl_getinfo( $ch );
    curl_close( $ch );
    $header['errno']   = $err;
    $header['errmsg']  = $errmsg;
    $header['content'] = $content;

    // Time (ms)
    $totaltime = $header['total_time']*1000;
    $totaltime = number_format($totaltime, 0, "", "");
    // Handle keyword search
    $izkey = 1;
    if(!empty($keyword)) {
        if (strpos($header['content'],$keyword) !== false) {
            $izkey = 1;
        }
        else {$izkey = 0;}
    }

    // Return
    return $header['http_code']."|".$totaltime."|".$izkey;
}

The function variables are as follow:

$proc: not user input, will either be http or https
$url: user input and will be a full link except the http:// or https:// part at the beginning
$keyword: user input, just alphanumeric characters
$method: not user input, will be GET
$timeout: user input, only digits, no more than 60
$maxredirs: user input, only digits, no more than 60

My question is: with the above mentioned user input and returning just 3 variables (see last line of code) is this function safe for the server to run like it is now? Could a user pass a malicious link to harm the server?

I know it may be a silly question, but, I want to be sure. It's better safe than sorry, and it can't hurt to ask.

A user will probably not be able to hurt your server. You do open your system up to SSRF which might cause a problem, depending on your system. There might also be a possibility to DOS your server as you keep connections open for 60 seconds, which is a rather long time (I'm not quite sure about this, but you should definitely test this).

You also provide your users the possibility to attack other systems using your server. They may exploit vulnerabilities in other systems that can be exploited via GET, and they may use your server to scan websites because you allow a keyword search. It may also aid in DOS attacks. You may or may not be liable for damages causes by this, I personally wouldn't risk it.

At a minimum, I would disable the keyword search, limit the amount of requests a user can do, and change the user agent to something more honest (eg MyAvailabilityChecker (more info: http://mywebsite.com/availabilitychecker), and provide an option to opt-out).

Regarding your code: It's generally good. A couple of points:

your comments don't add anything (except the comment about time in ms). And they are partly wrong (eg stop after 10 redirects). I would just remove them.
why proc? Shouldn't it be prot? Either way, don't shorten variable names, it makes code harder to read. Same goes for maxredirs. Also, use snake_case for all variables, so it might be max_redirects (max is commonly used, so shortening is ok).
else {$izkey = 0;} is not properly formatted.


Use of Factory pattern — how to implement it I am trying to figure out how Factory pattern works. I implemented it with TypeScript in this github repo: https://github.com/emilecollin/factory_pattern

Basically this is emulating spaceships gaming logic: the program will ask you to input in the terminal one of these options: "alien" or "zombie". With the provided input, the program will select on runtime the proper ship class to instantiate (AlienShip or ZombieShip), and a spaceships fight is mocked. The logic for the decision of which spaceship class to instantiate lives in a «factory method».

The abstract creator:

export abstract class AbstractShipFactory {
  public abstract makeShip(shipType: string): Ship;
}

The concrete creator:

export class EnemyShipFactory extends AbstractShipFactory {
  public makeShip(shipType: string): Ship {
    switch (shipType) {
      case 'alien':
        return new AlienShip();
      case 'zombie':
        return new ZombieShip();
    }
  }
}

The Product interface:

export interface Ship {
  name: string;
  damage: number;
  attack(): string;
}

And the Concrete Product class:

export class AlienShip implements Ship {
  private _name: string = 'alien';
  private _damage: number = 35;

  get name(): string {
    return this._name;
  }

  get damage(): number {
    return this._damage;
  }

  attack(): string {
    return '---->You are being attacked by ' + this.name + ' spaceship and got a damage of ' + this.damage;
  }
}

I'm now sure if I got it right. Still a bit confused about it. Any opinion of this code will be welcome!

There's no need to pass a string to makeShip. You should instead create 2 methods: makeAlienShip and makeZombieShip. This way as someone working on your code I can let my editor autocomplete and you won't have to write as much documentation.
As per above, having an AbstractShipFactory is an overuse of abstraction. What you could do is instead just make a ShipFactory that makes different types of ships: allied, enemy, neutral, zombie, pirate, alien etc.
Try to use template strings instead. It makes it much more readable especially if you're spitting out long messages. Like so:

return `---->You are being attacked by ${this.name} spaceship and got a damage of ${this.damage}`

Why is attack() responding with the string "You are being attacked by..."? This is extremely awkward and hard to understand: can a ship attack another ship? Or can a ship only attack the player? Maybe you could change the method name to takeDamageFromShip(). Even better would be to make a method attack(ship: Ship)

Think more about the relationships between objects in your program. You defined what a Ship is, but how does it interact with other objects in your program?

If zombie, alien, etc. are meant to be types of a Ship, add a string to your Ship interface instead.
Why are you not using a constructor to instantiate the properties of AlienShip? What if certain ships do more damage than others?


Creating objects from parsed file I'm newbie in Python but I want to know some tricks to make my code look cute. I know that syntax of Python allows to do this, but I don't know how.
Suppose I have file with lines of format "x y z" and I want to parse it and make some objects. Now code looks like this:

class Point(object):
    def __init__(self, x, y, z):
        self.x = x;
        self.y = y;
        self.z = z;

...

def load_file(file_path):
    points = [];
    with open(file_path, "r") as input_file:
        for line in input_file:
            str_point = line.split();
            x = float(str_point[0]);
            y = float(str_point[1]);
            z = float(str_point[2]);
            points.append(Point(x, y, z));
    return points;

Is there any way to make this code simpler?

Instead of parsing x, y, z one by one, you can use a list comprehension,
and pass the whole thing directly to the the constructor of Point using * like this:

def load_file(file_path):
    objects = []
    with open(file_path) as input_file:
        for line in input_file:
            obj = Point(*[float(x) for x in line.split()])
            objects.append(obj)
    return objects

I dropped the "r" in open(..., "r"), as that's the default anyway.
Even more importantly,
; are not required to end statements in Python,
and they are just noise in the posted code.

It would be even cuter to use a generator pattern instead:

def load_file(file_path):
    with open(file_path) as input_file:
        for line in input_file:
            yield Point(*[float(x) for x in line.split()])

You could iterate over this to do something with the objects, for example:

for obj in load_file(file_path):
    # do something ...

Or you could load all into a list using a list comprehension:

objects = [x for x in load_file(file_path)]


Simple JavaScript timer function I've written a timer to start when the page loads, and to stop when the user clicks a "finished" button:

<br><button id = "finish" onclick = "stopTimer();">Done</button>

My code works fine, but I was just wondering if there's a simpler/easier/faster way to implement a timer in JavaScript.

//Stop timer function
function stopTimer() {
    clearInterval(time);
}

//Start timer function
var secTime = 0,
      minTime = 0,
      hourTime = 0;

var time =  setInterval(function(){
    var maxSec = 59,
        maxMin = 59,
        maxHour = 59;
if(secTime > maxSec){
      minTime++;
      if(minTime > maxMin){
        hourTime++;
        if(hourTime > maxHour){
          hourTime = 0;
          minTime = 0;
          secTime = 0;
        }
        minTime = 0
      }
      secTime = 0;
    }
        var newSec = (secTime.toString().length == 1) ? '0' + secTime : secTime,
            newMin = (minTime.toString().length == 1) ? '0' + minTime : minTime,
            newHour = (hourTime.toString().length == 1) ? '0' + hourTime : hourTime;

        document.getElementById('timer').innerHTML = newHour + ':' + newMin + ':' + newSec;

    secTime++;
}, 1000);

Given the nature of the snippet, I'll just overlook the fact that you're using inline JS on your elements, and your script is entirely in the global namespace. Normally I'd recommend using something like addEventListener for your click events, and enclose your code in an IIFE.

Now to get that big elephant in the room, timers in JS are inaccurate. The delay in timers only guarantee minimum time until execution, but not exact time of execution. Your time will drift. What I would suggest is to take the time during page load. Then on set intervals, you get the current time, subtract the current time from the start time to get the time elapsed, and from that, compute your hours, minutes and seconds.

Now for the rendering, it doesn't matter how often you actually print to the page as long as you are using the time elapsed. Also, I would suggest putting the reference to the DOM element in a variable outside the interval. That way, you won't be fetching the element from the DOM every time (which is slow).

Your padding logic is also repeated. You could just put that in a function and call it when needed. You can use slice with a negative index to slice from the right. Also, since we're operating in milliseconds (because of Date.now()), and there's some division, your numbers will have decimals. To eliminate them, you can use | 0 to clip them off. It's somewhat the same as Math.floor.

Given that review, your code should look something like:

var startTime = Date.now();
var second = 1000;
var minute = second * 60;
var hour = minute * 60;
var container = document.getElementById('timer');

function stopTimer() {
    clearInterval(timer);
}

function pad(n){
  return ('00' + n).slice(-2);
}

var timer = setInterval(function(){
  var currentTime = Date.now();
  var difference = currentTime - startTime;

  var hours = pad((difference / hour) | 0);
  var minutes = pad(((difference % hour) / minute) | 0);
  var seconds = pad(((difference % minute) / second) | 0);

  container.innerHTML = hours + ':' + minutes + ':' + seconds;

// This only represents time between renders. Actual time rendered is based
// on the elapsed time calculated above.
}, 250);

<button type="button" onclick="stopTimer()">Stop</button>


Experience replay memory I have just begun learning Julia. Here is an implementation of experience replay memory for use in a reinforcement learning algorithm. It is pretty simple, essentially a ring buffer with the following requirements:

Used to store 1D arrays of numbers, typically Float32 or Float64. All the stored arrays are the same size.
Has a maximum capacity, after which new entries overwrite old ones
Has a sample function for retrieving a given number of entries

import Base.length

struct Memory{T <: Real}
   max_size::UInt32
   experiences::Vector{Vector{T}}
end

Memory{T}(max_size) where {T <: Real} = Memory{T}(max_size, Vector{Vector{T}}())

length(memory::Memory) = length(memory.experiences)

function remember!(memory::Memory, experience)
   size = length(memory)
   if size == memory.max_size
      memory.experiences[1 + size % memory.max_size] = experience
   else
      push!(memory.experiences, experience)
   end
end

function sample(memory::Memory{T}, count::Integer) where {T <: Real}
   size = length(memory)
   @assert count <= size
   return [memory.experiences[1 + rand(UInt32) % size] for i in 1:count]
end

There's a CircularBuffer type in DataStructures, which does pretty much the same.  Have a look at its code for some inspiration.  Or reuse it, if it fits your needs and you don't hesitate to add that dependency.
Regarding the point before, I'd especially recommend to implement the common interfaces, and probably iterate as well.  remember! makes sense semantically, but push! is the standard name for this functionality.  You could implement remember! in terms of push! and export both, or just use push! and mention it in the docs.
There isn't really a need to be so specific about types.  I'd just use

struct Memory{T}
   max_size::UInt32
   experiences::Vector{T}
end

Nothing is lost by generalizing in this way, since you never use information about the content.  But who knows, maybe you later want switch to different types for experiences, eg. StaticVectors.
I would doubt whether using UInt32 saves more than it complicates.  Int is pretty much standard for everything of this kind: lengths, indices, offsets, etc.
Depending on the sizes of your use case, it might be better to preallocate experiences as Vector{T}(undef, max_size) and use just indexing (and keeping track of the actual length), instead of push!ing until full -- see the DataStructures implementation.  If not, calling sizehint!(experiences, max_size) might be good, if you expect to always exhaust the full capacity.
Instead of providing sample, I'd recommend hooking into rand, thereby getting more methods for free.  In the simplest case, this just consists of defining

Random.rand(rng::Random.AbstractRNG, s::Random.SamplerTrivial{Memory{T}}) where {T} = rand(rng, s[].experiences)

which gives you all variations: rand(m), rand(m, N) (corresponging to your sample), rand(rng, m),  etc.  Especially the form with a provided RNG is relevant if you want to write reproducible experiments.  The rand! methods seem not to work by default, but can be added easily if you need them.  (If you switch to a preallocation + indexing solution, you need to make sure to sample only from @view experiences[1:length].)

And note that you can directly sample from an array a using rand(a).


Immutable C++ String class I have project where I will need to create lots of immutable strings. If I am using std::string, which has huge overhead - about 60-70% against const char *. On a 64-bit machine, the current implementation uses 8 bytes for the class + the const char * size. This is the same size as it would be with plain C.

I will also do optimization with union and will put small strings into the pointer itself.

I need to know if I am on right track.

#include <stdio.h>

#include <memory>
#include <string.h>

class String{
public:
    String(){};

    String(const char *s) : _data(__dup(s)){};

    String(const String & other) : String( other.c_str() ){};

    String(String && other) = default;

    String & operator=(String other){
        std::swap(_data, other._data);
        return *this;
    }

    const char *c_str() const{
        return _data.get();
    }

    int cmp(const String & other) const{
        if (c_str() == nullptr)
            return other ? -1 : 0;

        return strcmp(*this, other);
    }

    operator bool() const{
        return c_str();
    }

    operator const char *() const{
        return c_str();
    }

    bool operator == (const String & other) const{
        return cmp(other) == 0;
    }

    bool operator != (const String & other) const{
        return cmp(other) != 0;
    }

    bool operator > (const String & other) const{
        return cmp(other) > 0;
    }

    bool operator >= (const String & other) const{
        return cmp(other) >= 0;
    }

    bool operator < (const String & other) const{
        return cmp(other) < 0;
    }

    bool operator <= (const String & other) const{
        return cmp(other) <= 0;
    }

private:
    static char *__dup(const char *s){
        auto size = strlen(s);
        char *copy = new char[size];
        memcpy(copy, s, size);
        return copy;
    }

private:
    std::unique_ptr<char[]> _data;
};

void p(String &s){
    printf("%s\n", s ? (const char *) s : "[null]");
}

int main(){
    String s1;          p(s1);
    String s2  = { "hello" };   p(s2);
    String s3a = s2;        p(s3a);
    String s3  = std::move(s3a);    p(s3);
    s1 = s3;            p(s1);
    s1 = "hi";          p(s1);

    String a = "aaaa";
    String b = "bbbb";
    String c = "aaaa";

    printf("%s %s\n", a == c ? "Y" : "N", "Y");
    printf("%s %s\n", a == b ? "Y" : "N", "N");
    printf("%s %s\n", a != b ? "Y" : "N", "Y");
    printf("%s %s\n", a <  b ? "Y" : "N", "Y");
    printf("%s %s\n", a >  b ? "Y" : "N", "N");
    printf("%s %s\n", a <= b ? "Y" : "N", "Y");
    printf("%s %s\n", a >= b ? "Y" : "N", "N");
    printf("%s %s\n", a <= c ? "Y" : "N", "Y");
    printf("%s %s\n", a >= c ? "Y" : "N", "Y");

    return 0;
}

Design

In modern standard libraries, std::string uses short string optimization, which allows to avoid heap allocations at all for string shorter then ~20 chars (on 64-bit machines). So you really want to measure performance, I am not readily convinced that your string offers performance improvement. Of course, for your specific use pattern it might, but you have to measure.

__dup function

This is the most problematic part of the code.

Apparently, you forgot to return value from __dup method (now fixed).
You must allocate one char more and fill it with zero. Or copy size+1 characters from the source.
Identifiers with leading double underscores or one leading underscore and a capital letter are reserved by the standard. You must not use such an identifier. It is enough that you already have put it into private section.

After the corrections, the function could look like this:

static char *dup(const char *s){
    auto size = strlen(s);
    char *copy = new char[size + 1];
    memcpy(copy, s, size + 1);
    return copy;
}

Safety

Functions do not check for null pointers. This code will cause illegal memory access:

String x;
String y{x};

Style

You don't need semicolons after the member function definitions
I would recommend not putting methods on a single line even if they have empty bodies.
You can use C++ style headers <cstring> and <cstdio> instead of C-style ones.
I would recommend to implement an operator<<(std::ostream &, const String &) so you could use your string together with iostreams.
For testing, it is better to use a testing framework. Like boost.test, gtest, Catch etc.


CSS animation via JavaScript I call this animateElements function as part of an animation loop (inspired via Dave Gamache). Everything works as expected, but I would love to try and improve/optimize if possible.

var animation, translate, translateY, translateX, scale, rotate, opacity, display;
    animateElements = function() {
        //var animation, translateY, translateX, scale, rotate, opacity, display;

        //do{ //No performance improvement in chrome with neg-loop + ...havent tested others

        //var i = keyframesConverted[currentKeyframe].animations.length;
        //while(i--) {

        for (var i = 0; i < keyframesConverted[currentKeyframe].animations.length; i++) {
            animation = keyframesConverted[currentKeyframe].animations[i];

            //NOT MUCH IN IT: this "if" vs ternary, but fps was better on ternary.

            display = (typeof animation['display'] === 'undefined') ? null : animation['display'][1] || null;

            if (display !== 'none') {
                translateY = (typeof animation['translateY'] === 'undefined') ? 0 : calcPropValue(animation, 'translateY', 2);
                translateX = (typeof animation['translateX'] === 'undefined') ? 0 : calcPropValue(animation, 'translateX', 2);
                scale = (typeof animation['scale'] === 'undefined') ? "" : " scale(" + calcPropValue(animation, 'scale', 2) + ")";
                rotate = (typeof animation['rotate'] === 'undefined') ? "" : " rotateX(" + calcPropValue(animation, 'rotate', 1) + "deg)";
                opacity = (typeof animation['opacity'] === 'undefined') ? 1 : calcPropValue(animation, 'opacity', 2);

                translate = 'translate3d(' + translateX + 'px, ' + translateY + 'px, 0.01px)';

                $(animation.selector).css({
                    'transform': translate + scale + rotate,
                    'opacity': opacity,
                    'display': display //= (opacity < 0) ? 'none' : display//Todo: Potential for performance Improvement but would require major recode?
                });
            } else {
                $(animation.selector).css({ 'display': 'none' });
            }
        }//while(i--)
    }

    calcPropValue = function (animation, property, deciPlaces) {
        var value = animation[property];
        return (tweenEase(relativeScrollTop, value[0], (value[1] - value[0]), keyframesConverted[currentKeyframe].duration, (typeof animation['tween'] === 'undefined') ? null : animation['tween'][1] || null)).toFixed(deciPlaces);
    }

Concerns:

The use of typeof animation[...] as most likely the ternary will reference it twice a loop, for each animation property; hence, should I place it in a var?
When calling calcPropValue(), would there be a performance gain by just passing animation['...type'] in one go, rather than the 'animation' and then its 'type'?
As this function has no further use - except from where it is called - would it be an improvement just to move this function block into that animation loop directly, as I assume 'fewer calls to functions, the better'?

Your priority is to reduce dependency on expensive strings  :

Change all associative syntax, animation['x'] >>> dot notation, animation.x .
Change 'undefined' >>> undefined.
Avoid passing/returning strings to/from functions. They are passed by value (a function uses its own copy).
Pass back a number from calcPropValue() and do the toFixed() back in the caller statement.

Also :

The tween setting is the same for every call in the same turn of the for loop, therefore calculate it once and pass to calcPropValue().
Simplify testing of tween and display values.

Inside the for loop, assign :

display = (animation.display && animation.display[1]) || null;
tween = (animation.tween && animation.tween[1]) || null;

Don't forget to add tween to the var declaration.

Modify calcPropValue() as follows :

calcPropValue = function (value, tween) {
    return tweenEase(relativeScrollTop, value[0], (value[1] - value[0]), keyframesConverted[currentKeyframe].duration, tween);
}

Ternery's will become, eg :

translateX = animation.translateX ? calcPropValue(animation.translateX, tween).toFixed(2) : '0.00';

Then consider sacrificing readability for performance by not assigning what the terneries return. Instead write them directly into the expressions where the resultant strings get used, eg :

'transform': 'translate3d(' +
    (animation.translateX ? calcPropValue(animation.translateX, tween).toFixed(2) : '0.00') + 'px, ' +
    (animation.translateY ? calcPropValue(animation.translateY, tween).toFixed(2) : '0.00') + 'px, 0.01px)' +
    (animation.scale ? ' scale(' + calcPropValue(animation.scale, tween).toFixed(2) + ')' : '') +
    (animation.rotate ? ' rotateX(' + calcPropValue(animation.rotate, tween).toFixed(1) + "deg)" : ''),

When you're done, pass the code through jslint or jshint to help spot any bloomers.


Waiting for a lock to release with Thread.Sleep()? There's a follow-up: Waiting for a lock to release with ManualResetEvent and Quartz.

I've written a simple Lock-Mechanism which is saving the states of the locks in a database. Now I need to wait for a Lock to release or timeout after a certain amount of time (blocking the main thread, this is intentional). My implementation is looking like this:

/// <summary>Waits for the lock to be released within the given timeout.</summary>
/// <param name="lockName">The name of the lock.</param>
/// <param name="timeout">The timeout in seconds.</param>
/// <returns>True if the Lock was released.</returns>
public bool waitForLock(String lockName, Int32 timeout)
{
    // IsLocked(String) does query the database for the status
    while(locker.IsLocked(lockName) && timeout > 0)
    {
        Threading.Thread.Sleep(1000);
        timeout--;
    }

    return !locker.IsLocked(lockName);
}

I always get a little bit itchy if I have to use Thread.Sleep, but I'm not sure why.

Is something wrong with this design/approach and will it bite me later? Or is there a better way to handle this?

Edit: Further clarifications: I have full control over the locker and anything affiliated with it. The main intention for this is to make sure that certain operations will not overlap, each of these operations should be taking well less then 5 seconds. The load on the server is not really an issue, because the realistic maximum number of clients simultaneous waiting for a lock could be 10 (in my case).

The blocking of the thread is not as intentional is I have stated. The best thing would be if I could keep the message pumping still active, but I fail to see how I can achieve that goal in combination with polling the database (and without the need to introduce a timer with a callback).

Edit2: The locked part is not the database, the database is only holding the status of the locks. I'm using this locking system to make sure that no operation protected by this can execute at the same time on different client machines.

An easier answer might be to use a ManualResetEvent. You can block the current thread for a timeout period until another thread signals the ManualResetEvent.

http://philiprieck.com/blog/archive/2004/01/27/WaitHandleSample.aspx

http://www.yoda.arachsys.com/csharp/threads/waithandles.shtml

Basically, blocking thread calls mre.Wait(), processing thread works until it calls mre.Set(). No sleeping, none of this other stuff.

An important note, always Dispose of your ManualResetEvents. Leaking handles is a bad call, this object reaches out into the COM world.


Initializing an Android activity by reading a CSV file with image IDs, names, and descriptions I am using Java to create an Android app. I have the main activity read a CSV file, create objects, and then save them into a singleton class so the rest of the activities in the app can access them easily.

This is working perfectly fine but I am not sure if there are some other ways to achieve the same result that are considered better practice, or if perhaps my solution could cause issues down the line that I am not aware of.

MainActivity extends AppCompatActivity

@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_main);

    createItemObjects();
}

public void createItemObjects(){
    InputStream is = getResources().openRawResource(R.raw.items_csv);
    BufferedReader reader = new BufferedReader(new InputStreamReader(is, Charset.forName("UTF-8")));

    ArrayList<Item> items = new ArrayList<Item>();
    String line = "";
    try {
        while((line = reader.readLine())!=null) {
            String[] values = line.split(",");
            String img = values[0];
            int imgID = getResources().getIdentifier(img , "drawable", getPackageName());
            items.add(new Item(imgID, values[1], values[2]));
        }
    } catch (IOException e) {
        Log.v("Main Activity", "Error Reading File on Line " + line, e);
        e.printStackTrace();
    }
    ItemListHolder.getInstance().setItemsArrayList(items);
}

So as you can see, when the app launches it reads the CSV file, creates Item objects, creates an ArrayList to store those Item objects, then sends it into the ItemListHolder singleton, so that all the Activities in the app can use ItemListHolder.getInstance().getItemsArrayList() to retrieve the ArrayList of Item.  Each Item is constructed with an image ID, name, and description from a line in the CSV file.

This works without any issues, but my question is: Is having your Android app create objects from a CSV file everytime it restarts considered good practice/safe to use/efficient ect, or should you be storing your objects in some other way, such as shared preferences, so that they are actually saved in memory and do not need to be created everytime the app launches and then held inside a class.

I am familiar with Java but am very new to Android so I really have no idea what other ways of saving objects may or may not exist, or what is considered good to use.  Is it better to save them to the fileOutputStream? Is it better to save them to sharedPreferences? Is there some other storage method that is better?

People from Android have a really comprehensive documentation about it at https://developer.android.com/guide/topics/data/data-storage.

Short answer: yes, there are better ways: SharedPreferences for simple data; Local database (SQLite) for complex data. Both of them can be persistent.


Parsing a long without using inbuilt function parseLong I came across it as a problem to solve. What kind of limitations are there?

public long parseLong(String number)
{
    long n=0;
    if(number==null || number.isEmpty())
    {
        System.out.println("Number is null or empty");
        return n;
    }
    boolean isNegative = false;
    if(number.charAt(0)=='-')
    {
        isNegative=true;
    }
    else {
        n = getValue(number.charAt(0));
    }
    for(int i=1;i<number.length();i++)
    {
        n = n*10 + getValue(number.charAt(i));
    }
    return isNegative ? (-1*n) : n;
}
public int getValue(char ch)
{
    int value = ch;
    if(value<'0' || value>'9')
    {
        throw new NumberFormatException();
    }
    return value-'0';
}

If the condition if(number==null || number.isEmpty()) holds true the method should throw a ParseException rather than incorrectly returning 0.

Also your logic for non-negative numbers isn't quite correct, as you are ignoring the first character if the length of the input is > 1. To fix this you should include charAt(0) in your else statement as follows:

    if(number.charAt(0)=='-'){
        isNegative=true;
    }
    else {
        n = getValue(number.charAt(0));

        for(int i=1;i<number.length();i++){
            n = n*10 + getValue(number.charAt(i));
        }
    }

Also in your getValue method your check, whether the character is a digit can be simplified:

public int getValue(char ch){
    if (Character.isDigit(ch)){
        return ch - 48;
    } else {
        throw new NumberFormatException();
    }
}

These are local optimizations. I would consider converting the string to an array of integers first (and by doing that determine, whether the string is valid in the first place) and then compute the value. By doing the computations as you go you are risking doing them in vain if a later input character is not a digit.

One thing that is missing in your code is considering the maximum value for a long and checking for violations.


Anagram substrings in a string I was trying to solve this questions:

Given an input string, find all combination of unique substrings, which are anagrams of each other and distinct from each other. The substrings must follow 2 <= length(substring) < len(original string).

Example 1:

String: “vadapav”
substrings: va, vad, vada, vadap, vadapa, ad, ada, adap, adapa, adapav, da, dap, dapa, dapav, ap, apa, apav, pa, pav, av
Output: (va, av), (ad, da), (ap, pa)

How can it be optimised in terms of algorithmic and code complexity?

# Code goes below
from collections import defaultdict
import itertools
def anagram_substring(str):
    substr_list = []
    ans = []
    is_present = defaultdict(list)
    for i in xrange(len(str)):
            for j in xrange(i+2, len(str)+1):
                substr_list.append(str[i:j])

        substr_list = list(set(substr_list))
    for substr in substr_list:
        if is_present[''.join(sorted(substr))]:
                    for anagram_str in is_present[''.join(sorted(substr))]:
            ans.append([anagram_str,substr])
        is_present[''.join(sorted(substr))].append(substr)
    return ans

str = raw_input().strip()
print anagram_substring(str)

There's no need to use a list and convert it to a set, you can use a set from the start.

Also, you're checking for every entry if it's in the list as an anagram and adding it to the answer, which is inefficient. You can do this in a faster way by using a dictionary that has the sorted substr as key and pushing the values in there.

from collections import defaultdict
import itertools

def anagram_substring(str):
    substr_list = set([])
    anagrams = defaultdict(list)

    for i in xrange(len(str)):
        for j in xrange(i + 2, len(str) + 1):
            substr_list.add(str[i:j])

    for substr in substr_list:
        anagrams[''.join(sorted(substr))].append(substr)

    return [values for key, values in anagrams.iteritems() if len(values) > 1]

str = raw_input().strip()
print anagram_substring(str)


SEDE-Query for Rep-Trivia The code should be relatively self-explanatory. I was wondering how much rep the Top Voters on sites did "generate" on a site, in comparison to how much rep they "own".

Therefore I created a little SEDE-Query and I am now wondering, if I follow good SQL-Practices and how the performance of the query could possibly be improved (even though it's rather simple already..)

WITH Voters AS(
   SELECT DISTINCT UpVotes, DownVotes, Id, Reputation
   FROM Users
   WHERE Reputation != 101 AND Reputation != 1 --Exclude users without repchanges
   )

SELECT TOP ##topX:int?20##
    Users.Id as [UserLink],
    Users.DisplayName as [SortName],
    Voters.Upvotes * 8 as [EstimatedAddedRep],
    --as there is no split between question and answer votes
    -- we take 8 as median in accordance to the tendency that there are more
    -- answer votes (10 rep) than question votes (5 rep)
    Voters.Downvotes * 3 as [EstimatedDestroyedRep],
    Users.Reputation as [OwnedRep],
    Voters.Upvotes * 8 / Users.Reputation as [Ratio]
FROM Users, Voters
WHERE users.id = Voters.Id
GROUP BY Users.Id, Users.DisplayName, Users.Reputation, Voters.UpVotes, Voters.DownVotes
ORDER BY EstimatedAddedRep DESC

For anyone who wants to run the newest query, you can do so here: Rep "Created" vs Rep "owned" by topX voters

First of all, when you are selecting and want to perform mathematics on it you should directly specify what you want done

instead of

Voters.Upvotes * 8 as [EstimatedAddedRep],

it should be

(Voters.Upvotes * 8) AS [EstimatedAddedRep]

And

Voters.Upvotes * 8 / Users.Reputation as [Ratio]

Should be

((Voters.Upvotes * 8) / (Users.Reputation)) as [Ratio]

you should always specify with parenthesis when performing arithmetic in code, because it doesn't always do what you think it will do.

I don't do this with my FROM and WHERE Statements

FROM Users, Voters
WHERE users.id = Voters.Id

I would do this

FROM Users INNER JOIN Voters ON Users.Id = Voters.Id

and leave out the WHERE clause.

For the issue brought up in Chat  you would just need to add it in like this (simple query so not an issue to repeat yourself in the select)

((Voters.Upvotes * 8) / Users.Reputation) AS RepCreated --?????

Check all your Columns and Tables as well, you have some weird casing issues where sometimes you capitalize and sometimes you don't, it was hard to figure out what was right


Optimized Longest Collatz Sequence I'm writing a program to calculate the longest collatz sequence for positive integers below some N. I have optimized it as far as I can - is there anything more I could do to optimize it? Any constructive criticism is welcome :)

A few interesting notes: It is easy to prove (by contradiction or otherwise) that the number that produces the longest chain exists in the second half of the numbers from 1-N (that is, it must be greater than N/2). If we try to run the loop from N/2 to N, the running time becomes substantially slower. Interesting... Also, the lower while loop is an interesting idea, but provides no tangible performance benefits (or losses, for that matter - which is why I kept it. It's an interesting idea).

Thank you!

#include <iostream>
#include <chrono>

using namespace std::chrono;

const int N = 1000000;
long long iCpy;
short ct;
short maxLength = 0;
short vals[N] = {0};

int main() {
  // Time the program:
  auto startTime = steady_clock::now();

  int i, maxIndex = 0;

  // calculate collatz lengths for all indices.
  // keep track of max value:
  for (i = 2; i < N; i++) {
    iCpy = i;
    ct = 0;

    while (iCpy != 1) {
      if (iCpy < N && vals[iCpy]) {
        ct += vals[iCpy];
        break;
      }

      if (iCpy & 1) {
        iCpy *= 3;
        iCpy++;
      } else {
        iCpy >>= 1; // Equivalent to iCpy /= 2
      }

      ct++;
    }

    iCpy = i;
    vals[iCpy] = ct;
    while (iCpy < N/2) {
      iCpy <<= 1; // Equivalent to iCpy *= 2
      vals[iCpy] = ++ct;
    }
    if (ct > maxLength) {
      maxLength = ct;
      maxIndex = iCpy;
    }
  }

  // output the max index and length:
  std::cout << "Max number: " << maxIndex << ", with length " << maxLength << std::endl;

  auto elapsed = duration_cast<milliseconds>(steady_clock::now() - startTime);
  std::cout << "Program execution took " << elapsed.count() << "ms\n";
  return 0;
}

Here are some suggestions:

Don't use global variables.
Instead of using a single big function, break it down to logical parts.
const int N = 1000000; should be constexpr instead of const.
Instead of = {0}, use {}.  The latter makes it clear that you are
value initializing all elements.
i should be declared in the loop.
Post-increment operators i++ should be changed to pre-increment
++i in a discarded-value expression.
iCpy & 1 should be iCpy % 2 == 0.  iCpy <<= 1; // Equivalent to iCpy *= 2 should be iCpy *= 2.  Optimization doesn't happen in this way — the compiler knows such things much better.
Use << '\n' instead of << std::endl when you don't need the flushing behavior.