Password and confirm password validation in Android Studio I just wrote this password and password confirmation validation code function that checks to see if a password is empty or not, and is it match (password = confirmPassword). I'm not sure this is the best way to do it, but for now it dont works really well.

private void checkDataEntered()
{
    if(isEmpty(RegisterFirstName))
    {
        RegisterFirstName.setError("You must enter first name to register");
    }

    if(isEmpty(RegisterLastName))
    {
        RegisterLastName.setError("Last name is required");
    }

    if(isEmail(RegisterEmail) == false)
    {
        RegisterEmail.setError("Enter your valid email.");
    }

    if(isEmpty(RegisterPassword))
    {
        RegisterPassword.setError("Enter your password.");
    }

    if(isEmpty(RegisterConfirmPassword))
    {
        RegisterConfirmPassword.setError("Enter your confirmation password");

        if (!RegisterConfirmPassword.equals(RegisterPassword))
        {
            Toast.makeText(Signup.this, "Password do not match", Toast.LENGTH_SHORT).show();
        }
    }

    else
    {
        loadingBar.setTitle("Creating New Account");
        loadingBar.setMessage("Please wait while we are creating account for you.");
        loadingBar.show();

        String email = null;
        String password = null;

        mAuth.createUserWithEmailAndPassword(email, password).addOnCompleteListener(new OnCompleteListener<AuthResult>()
        {
            @Override
            public void onComplete(@NonNull Task<AuthResult> task)
            {
                if(task.isSuccessful())
                {
                    Toast.makeText(Signup.this, "You have successfully signed up", Toast.LENGTH_SHORT).show();
                    Intent mainIntent = new Intent(Signup.this, MainActivity.class);
                    mainIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK | Intent.FLAG_ACTIVITY_CLEAR_TASK);
                    startActivity(mainIntent);
                    finish();
                }
                else
                {
                    Toast.makeText(Signup.this, "Error occured, please try again.", Toast.LENGTH_SHORT).show();
                }
                loadingBar.dismiss();
            }
        });
    }
}

Inconsistent UX

Most validation errors call setError of the relevant input field.
An exception is when the confirmation password doesn't match the first entry,
that pops up a toast.
It would be better to handle validation errors consistently,
by using setError for mismatched confirmation password.

It's ok to use a toast for the result of the registration,
when all form fields are valid.

Creating account with invalid input

Before calling mAuth.createUserWithEmailAndPassword,
the only validation that's really enforced is that the confirmation password is not empty.
That's strange.
It would be better to call this only when all entries of the form are filled and valid.

Style issues

A convention in Java is to use camelCase for variable names.
For example RegisterConfirmPassword should be registerConfirmPassword.

Instead of if (a == false) { it's more natural to write if (!a) {.

It's a convention in Java to place braces like this:

if (cond) {
    // ...
} else {
    // ...
}


Compute a numerical derivative Since I could not get numpy.gradient() to compute a derivative successfully, I wrote a script to compute it manually. Running the script below will output a plot of two functions f(x) = sin(x) and f'(x) = cos(x) over the interval 0 ≤ x ≤ 2 pi.

import numpy as np
import matplotlib.pyplot as plt

def compute_numerical_derivative(func, x, method='custom'):
    y = func(x)
    if method == 'custom':
        size = len(y0)
        res = np.zeros(size, 'd') # 'd' for double
        # centered differences
        for idx in range(1, size-1):
            res[idx] = (y[idx+1] - y[idx-1]) / (x[idx+1] - x[idx-1])
        # one-sided differences
        res[0] = (y[1] - y[0]) / (x[1] - x[0])
        res[-1] = (y[size-1] - y[size-2]) / (x[size-1] - x[size-2])
    # elif method == 'numpy':
    #     res = np.gradient(y)
    return res

x = np.linspace(0, 2*np.pi, 100)
y0 = np.sin(x)
y1_true = np.cos(x) # exactly d/dx (y0)

y1_cust = compute_numerical_derivative(np.sin, x, method='custom')
# y1_nump = compute_numerical_derivative(np.sin, x, method='numpy')

plt.plot(x, y1_true, 'r', marker='o', linestyle=None, alpha=0.5)
plt.plot(x, y1_cust, 'b', marker='^', linestyle=None, alpha=0.5)
# plt.plot(x, y1_nump, 'k', marker='*', linestyle='-', alpha=0.5)
plt.show()

Can the speed and/or accuracy of this algorithm be improved? Is it proper to use centered differences at interior points and one-sided differences at the boundaries?

You can use np.roll to compute the centered differences as a vectorised operation rather than in a for loop:

res = (np.roll(y, -1) - np.roll(y, 1)) / (np.roll(x, -1) - np.roll(x, 1))

You then still need to account for incorrect values at the boundaries.

Also note that you have a bug at the line

size = len(y0)

It should be len(y). But you don't need it anymore as you can access the last values as y[-1] or x[-2].

Lastly, consider putting your test code under an if __name__ == '__main__' clause.


Generating all subsets of a given set I am learning Java Collections Framework. Can someone look at this code for generating all subsets of a given set and tell me any issues with it.

import java.util.*;
public class AllSubsets {
    public static void main(String[] args) {
        Set<Integer> original = new HashSet<Integer>();
        for (int i = Integer.parseInt(args[0]); i > 0; i--) {
            original.add(i);
        }
        System.out.println(generateAllSubsets(original));
    }

    public static HashSet<HashSet<Integer>> generateAllSubsets(Set<Integer> original) {

        HashSet<HashSet<Integer>> allSubsets = new HashSet<HashSet<Integer>>();

        allSubsets.add(new HashSet<Integer>()); //Add empty set.

        Iterator it = original.iterator();
        while(it.hasNext()) {
            Integer element = (Integer) it.next();

            //Deep copy all subsets to temporary power set.
            HashSet<HashSet<Integer>> tempClone = new HashSet<HashSet<Integer>>();
            for (HashSet<Integer> subset : allSubsets) {
                tempClone.add((HashSet<Integer>)subset.clone());
            }

            //All element to all subsets of the temporary power set.
            Iterator it2 = tempClone.iterator();
            while(it2.hasNext()) {
                Set<Integer> s = (HashSet<Integer>) it2.next();
                s.add(element);
            }

            //Merge both power sets.
            allSubsets.addAll(tempClone);
        }
        return allSubsets;
    }
}

The first, obvious, issue is that generating the set in memory uses a lot of memory. However, making a lazy version is a bit more advanced, so don't worry about it for now.

1. Code to the interface, not the implementation

public static HashSet<HashSet<Integer>> generateAllSubsets(Set<Integer> original) {

is bad.

public static Set<Set<Integer>> generateAllSubsets(Set<Integer> original) {

is better.

2. Generics reduce repetition

public static <T> Set<Set<T>> generateAllSubsets(Set<T> original) {

is better still, because you can use it to find power sets of any set.

3. Generics remove the necessity to do most casts

    Iterator it = original.iterator();
    while(it.hasNext()) {
        Integer element = (Integer) it.next();

and

        Iterator it2 = tempClone.iterator();
        while(it2.hasNext()) {
            Set<Integer> s = (HashSet<Integer>) it2.next();

have wholly unnecessary explicit casts (which in the second case generates a warning) because you're using the raw type Iterator. Both can also be simplified using the same foreach syntax which you use when making the copy of the power set so far.

4. KISS

You have three loops (including the one hidden behind allSubsets.addAll), which seems to me to be unnecessarily complicated. If you make a shallow copy first then you can combine the deep copy with the adding an element.

public static <T> Set<Set<T>> generateAllSubsets(Set<T> original) {
    Set<Set<T>> allSubsets = new HashSet<Set<T>>();

    allSubsets.add(new HashSet<T>()); //Add empty set.

    for (T element : original) {
        // Copy subsets so we can iterate over them without ConcurrentModificationException
        Set<Set<T>> tempClone = new HashSet<Set<T>>(allSubsets);

        // All element to all subsets of the current power set.
        for (Set<T> subset : tempClone) {
            Set<T> extended = new HashSet<T>(subset);
            extended.add(element);
            allSubsets.add(extended);
        }
    }

    return allSubsets;
}

You'll note that I'm using the copy-constructor rather than clone(). That's a matter of taste: fundamentally there's no real difference, because they're both special-cased.


Number-guessing game with a cheat code As my first Java program, I have written a number-guessing game. I would appreciate any suggestions on how I can clean it up or streamline it. I don't mean cut down on the general tasks executed within the program, but rather if there were better methods/classes/syntax I could have used to make it better. I took a few snippets from a tutorial.

import java.util.Scanner;
import java.util.Random;

class Game {

public static void main(String[] args) {

  Scanner sc = new Scanner(System.in);
  //generate a random number
  //take user input
  //check the input act appropriately
  boolean running = true;
  boolean guessed = true;
  boolean realInt = true;

  int number = 0;
  int input = 0;

  String firstName=" ";

   System.out.print("want to play the game? Yes/No: ");
   firstName = sc.next();

  while (running) {

    if (guessed && (("yes".equals(firstName)) | ("cheater".equals(firstName)))){

        System.out.println("I have chosen a random value");
        System.out.println("Have a go at guessing it");

        Random rand = new Random();
        number = rand.nextInt(10) + 1;

        guessed = false;

    }

     if(("yes".equals(firstName)) | ("cheater".equals(firstName)))
     {

          if(sc.hasNextInt()) {
            realInt=true;
            input = sc.nextInt();
          }
          else{
            System.out.println("That is not an integer.");
            running = false;
          }

        }

     else
      running = false;

     if((((input != number) && realInt)&&(input != -1))){
           System.out.println("Plsease try again");
         }

    //below line sets up when to stop the program, when -1 is entered
         if (input == -1){
            running = false;
         }
         else{
           guessed = input == number;
         }

    //below lines are cheat codes

    if (input == -5){
        System.out.println("Answer: " + number);
    }

   if ("cheater".equals(firstName) && (input != number)){
            System.out.println("Answer: " + number);

      }

   }

 }

}

Depends on how you want to improve it, there's number of ways to go. For instance:

Procedural vs OO code - Extendability

If you wish to extend your game, add new functionalities, make it one game among many that users can play, you'd better switch from current programming style to more modularized one.

Right now that code is contained within "main" method. It's quite common for first programs, to be like that. It's rather procedural programming - you give the computer instruction after instruction. While easy to follow, it's problematic when you try to expand such code or maintain it. It's also mind-numbing to read, making it hard to modify properly.

You may want to modularize it somewhat:

group instructions together into procedures / functions
more related procedures into classes
leave in main as few things as you can

This is obviously very short answer to very large topic, but do ask if you're interested. You would then have to model the program using objects.

For me that would be number 1. Even over syntax.

Syntax & Cleaning

Tools help

Intellij IDEA has a menu called Analyze. If you select Inspect code there, you'll notice a few pointers, along with explanation.

Additionally, there is Checkstyle and FindBugs, they also offer lots of pointers.

Heed earlier advice

Giving credit where it's due, I also completely agree with sje397's comment. Let me quote it here:

  Fix indenting. Fix variable names (firstName isn't holding a first
  name). You don't even need to enter the loop if the answer is 'no' or
  the value us 'cheater'. The | symbol is 'binary or' - you want ||. You
  can use boolean vars to store the results of your calls to equals.
  That's a start :)

Less sparse code

Less empty lines. Your code is very sparse which makes it harder to read.

Initializing many variables need not happen in different lines.

    boolean running, guessed, realInt;
    running = guessed = realInt = true;

Rename number to answer.

It's not just a number, it's a game answer.

Read on String comparison

You're using equals for it. There's also equalsIgnoreCase which in most cases is preferred.

Yoda conditionals?

Some folks don't like Yoda-conditionals.

"value".equals(variable) -> variable.equals("value").

Choose your own flavour.

DRY

Keep your code DRY, that is Don't Repeat Yourself. Often you use same conditions in your flow-control instructions (if, while...).

Cheat codes

You have two cheat codes. One is to input -5, the other is to answer starting question with "cheater". Both serve same purpose, the game reveals the number. If so, why not collapse to ifs into one? Because the code executed in is the same.

Checking user input

Note: this code is changed using pointers above.

| -> ||
equals -> equalsIgnoreCase
less parens, cut those redundant

You may want to shorten them to functions. This is easy to do in IDE. Select ("yes".equalsIgnoreCase(firstName) || "cheater".equalsIgnoreCase(firstName)) and - if you use Eclipse, press Alt+T, this opens refactoring window. Choose extract method, name your method 'yesOrCheater' and proceed. In Intellij the shortcut is Ctrl+Alt+M.

Then

if (guessed && ("yes".equalsIgnoreCase(firstName) || "cheater".equalsIgnoreCase(firstName))) {

becomes

if (guessed && yesOrCheater(firstName)) {

Usability

Here is my game log:

want to play the game? Yes/No: yes
I have chosen a random value
Have a go at guessing it
5
Plsease try again
4
Plsease try again
65

# [some tries later]

Plsease try again
3
I have chosen a random value
Have a go at guessing it
2
[...]
Plsease try again
5
I have chosen a random value
Have a go at guessing it

Firstly... please. If I'm to read that word that often, please, write it well. ;-)

Also, how can I quit? I'm bored, I want to quit.

When I guessed right, don't just skip to next game, let me know it wasn't an error in the code, that I "won". ;-)

Finally, random value is a very broad term. Decimals? Negatives? 1:10? -100 gazillions : 98 gazillions? When I read this, I had unpleasant feeling about it, how long do I need to guess?

Testability

You may want to add tests or verifications that your game progresses as you intend it to. Using assert with JVM flag, or some test library (TestNG over JUnit, my personal preference). But that's also a large topic, perhaps for next time.


Taking YouTube links out of a list of GitHub repo READMEs I have a .csv file that contains student GitHub repository assignment submissions. I made a script to go to each repository and extract the YouTube video that they must have provided in their README file.

The structure of the CSV file is as follows:

Timestamp,Name,Student Number,Git Repo link

#!/usr/bin/python3

import csv
import github3
import time
import re
import argparse
from secrets import username, password

# API rate limit for authenticated requests is way higher than anonymous, so login.
gh = github3.login(username, password=password)
# gh = github3.GitHub() # Anonymous

def parse_args():
    parser = argparse.ArgumentParser()
    parser.add_argument("filepath", type=str, metavar="filepath", help="Filepath to the input csv file.")

    args = parser.parse_args()
    args = vars(args)  # Turn into dict-like view.

    return args

def get_row_count(filename):
    with open(filename, 'r') as file:
        return sum(1 for row in csv.reader(file))

def get_repositories(link):
    if gh.rate_limit()['resources']['search']['remaining'] == 0:
        print("API rate exceeded, sleeping for {0} seconds.".format(gh.rate_limit()['resources']['search']['reset'] - int(time.time()+1)))
        time.sleep(gh.rate_limit()['resources']['search']['reset'] - int(time.time()+1))

    return gh.search_repositories(link.replace("https://github.com/", "", 1), "", 1)

def main():
    filepath = parse_args()['filepath']
    if not filepath.endswith('.csv'):
        print("Input file must be a .csv file.")
        exit()

    p = re.compile(r"http(?:s?):\/\/(?:www\.)?youtu(?:be\.com\/watch\?v=|\.be\/)([\w\-\_]*)(&(amp;)?[\w\?=]*)?") # From http://stackoverflow.com/a/3726073/6549676
    row_counter = 0
    row_count = get_row_count(filepath)

    with open(filepath, 'r') as infile, open(filepath[:3] + "_ytlinks.csv", "w") as outfile:
        reader = csv.reader(infile)
        next(reader, None)  # Skip header

        writer = csv.writer(outfile)
        writer.writerow(["Youtube Link", "Name", "GitHub Link"])  # Write header

        for row in reader:
            for repo in get_repositories(row[3]):
                readme = repo.repository.readme().decoded
                if not readme:
                    readme = "No Youtube link found."

                if type(readme) is bytes:
                    readme = readme.decode('utf-8')

                ids = p.findall(readme)
                if len(ids) != 0:
                    ids = ids[0]

                ids = [x for x in ids if x]

                for _id in ids:
                    writer.writerow(['https://www.youtube.com/watch?v={0}'.format(_id), row[1], row[3]])

                if len(ids) == 0:
                    writer.writerow(['No Youtube Link Found', row[1], row[3]])

            print('Processed row {0} out of {1}'.format(row_counter, row_count))
            row_counter += 1

    print("Finished.")

if __name__ == "__main__":
    main()

Here are some concerns/suggestions:

you are reading the file twice - once to get the row count and when reading the links. And, you don't need to initialize the csv.reader to get the row count, simply use sum() over the lines in the file. You would probably need to use infile.seek(0) after getting the count and before initializing the csv reader
use _ for the throw-away variables (when counting the number of lines)
if len(ids) == 0: can be simplified as if not ids:
it looks like you don't need .findall() and should use .search() method since you are up to a single match
if there is a single repository link per line, you probably should have get_repository() method instead of get_repositories() and avoid the for repo in get_repositories(row[3]): loop - remember, "Flat is better than nested"
instead of handling the enumeration with row_counter manually, use enumerate()
instead of accessing the current row fields by index - e.g. row[1] or row[3], you can unpack the row in the for loop, something like (an example, I don't know your actual CSV input format):

for index, username, _, github_link in reader:

Or, you can use a csv.DictReader - accessing the fields by column names instead of indexes would improve readability - e.g. row["github_link"] instead of row[3]
you don't have to convert the args to a dictionary - return args and then access the arguments using a dot notation - e.g. args.filepath


Calculate IP Address I created this based on this website which calculates IP address information such as hosts, netmasks etc.

I'm quite new to C and I'm still grasping the idea of the whole array and pointers and bit manipulations. Code works the way I want it to work but I'd like to improve this. I would like to know how I did as well as what are some things I can look out for when programming in C.

#include <stdio.h>
#include <errno.h>
#include <string.h>
#include <stdlib.h>
#include <math.h>

#define B1 0
#define B2 1
#define B3 2
#define B4 3
#define CIDR 4

void ConvertIpToInt(char address[], int address_block[]);

int main(int argc, char** argv) {

    int address_block[4], netmask_block[4];

    char address[] = "128.42.0.0/25";

    ConvertIpToInt(address, address_block);

    char literal_netmask_bin[32];
    int bit_counter, net_bit_counter = 0;
    while (bit_counter != 33) {
        while (net_bit_counter != address_block[CIDR]) {
            literal_netmask_bin[bit_counter] = '1';
            net_bit_counter += 1;
            bit_counter += 1;
        }
        literal_netmask_bin[bit_counter] = '0';
        bit_counter += 1;
    }

    int i, block_counter, sqN_value, sqN = 0;
    bit_counter = 0;
    for (i = 0; i < 33; i++) {
        bit_counter += 1;
        if (literal_netmask_bin[i] == '1') {
            sqN_value += pow(2, 7 - sqN);
        }
        sqN += 1;
        if (bit_counter == 8) {
            netmask_block[block_counter] = sqN_value;
            bit_counter = 0;
            sqN = 0;
            sqN_value = 0;
            block_counter += 1;
        }
    }

    int wildcard_netmask_block[4];
    i = 0;
    for (i = 0; i < 4; i++) {
        wildcard_netmask_block[i] = ~netmask_block[i] & 0xFF;
    }

    printf("> [GENERAL]\n\n");
    printf("Address:---------------------> %d.%d.%d.%d\n", address_block[B1], address_block[B2], address_block[B3], address_block[B4]);
    printf("Network Mask:----------------> %d.%d.%d.%d => %d\n", netmask_block[B1], netmask_block[B2], netmask_block[B3], netmask_block[B4], address_block[CIDR]);
    printf("Wildcard Mask:---------------> %d.%d.%d.%d\n", wildcard_netmask_block[B1], wildcard_netmask_block[B2], wildcard_netmask_block[B3], wildcard_netmask_block[B4]);
    printf("Network Address:-------------> %d.%d.%d.%d\n", address_block[B1] & netmask_block[B1], address_block[B2] & netmask_block[B2], address_block[B3] & netmask_block[B3], address_block[B4] & netmask_block[B4]);
    printf("Broadcast Address:-----------> %d.%d.%d.%d\n", wildcard_netmask_block[B1] | address_block[B1], wildcard_netmask_block[B2] | address_block[B2], wildcard_netmask_block[B3] | address_block[B3], wildcard_netmask_block[B4] | address_block[B4]);
    printf("Minimum Usable Address:------> %d.%d.%d.%d\n", address_block[B1] & netmask_block[B1], address_block[B2] & netmask_block[B2], address_block[B3] & netmask_block[B3], (address_block[B4] & netmask_block[B4]) + 1);
    printf("Maximum Usable Address:------> %d.%d.%d.%d\n", wildcard_netmask_block[B1] | address_block[B1], wildcard_netmask_block[B2] | address_block[B2], wildcard_netmask_block[B3] | address_block[B3], (wildcard_netmask_block[B4] | address_block[B4]) - 1);
    printf("Number of Hosts:-------------> %d\n", (int) pow(2, 32 - address_block[CIDR]) - 2);
    printf("Total Hosts:-----------------> %d\n\n", (int) pow(2, 32 - address_block[CIDR]));
}

void ConvertIpToInt(char address[], int address_block[]) {
    char * pch;
    char delimeters[] = "./";
    pch = strtok(address, delimeters);
    int i = 0;
    while (pch != NULL) {
        address_block[i] = strtol(pch, NULL, 10);
        i += 1;
        pch = strtok(NULL, delimeters);
    }
}

Initialize before using

    int bit_counter, net_bit_counter = 0;

There are a couple problems.  First, if you declare and initialize in the same statement, you should only declare one variable.

    int bit_counter = 0;
    int net_bit_counter = 0;

If you have two variables, declare them on two lines.  Only declare multiple variables on a single line if you initialize none of them.  And some would argue not even then.  There's no functional difference, but it is much easier to read with every initialized declaration on a separate line.

Second, you don't initialize bit_counter.  C does not automatically initialize variables to default values, so this can cause bizarre behavior.  When I first tried to run this code, it gave me a runtime error until I initialized all the variables.

Know your bounds

    while (bit_counter != 33) {

This is a bit fragile.  You currently allow bit_counter to be incremented multiple times per iteration of this loop.  What if when it is 32, you increment to 34 without hitting this check?  The check will keep succeeding until the the program crashes (possibly due to trying to write outside the array).

    while (bit_counter <= 32) {

This is much more robust.  It no longer has to hit the mark exactly to stop.  Now it will stop for any value greater than 32, not just 33.

Another benefit, for readability, is that it now says 32, which is the critical number here.  Previously it said 33, which is one more than the last number.

But this is actually a bug.  It should be

    while (bit_counter < 32) {

because literal_bit_mask is only 32 long.  The first element is 0.  The last element is 31.  Now it won't write past the end of the array.

C has an increment by one operator

            net_bit_counter += 1;
            bit_counter += 1;

You can write this more idiomatically as

            ++net_bit_counter;
            ++bit_counter;

This shouldn't make much functional difference (perhaps not any on many platforms), but most C programmers would find this more recognizable.

I used the prefix form because some C compilers may not be smart enough to avoid the copy step of the postfix form.  I would expect most compilers to optimize that out, but we don't need to rely on that.  It's unlikely to make a noticeable difference here.

++bit_counter increments bit_counter and returns that value.  bit_counter++ returns the value it had before incrementing.  If you don't use the value (as you don't here), there is no real functional difference.  It's possible that the compiler may return different machine instructions.  In particular, it might copy the value to another register and then increment.  That's unnecessary if the

The following are functionally equivalent (leave the variables with the same values at the end):

int var = bit_counter++;

and

int var = bit_counter;
bit_counter++;

and

int var = bit_counter;
++bit_counter;

The following are different from the previous three, but are functionally equivalent to each other:

bit_counter++;
int var = bit_counter;

and

++bit_counter;
int var = bit_counter;

and

int var = ++bit_counter;

If bit_counter starts as 0 and is incremented to 1, then the first three will set var to 0.  The last three will set var to 1.

Many consider it easier to read if the variables are changed on separate lines.  This makes it obvious that the variables are changing.  Doing two things on the same line is less obvious, because people may see one and miss the other.


Mirror a stack of Integers Write a method mirror that accepts a stack of integers as a
  parameter and replaces the stack contents with itself plus a mirrored
  version of itself (the same elements in the opposite order). For
  example, suppose a variable s stores the following elements:

  bottom [10, 50, 19, 54, 30, 67] top

  After a call of mirror(s);, the stack would store the following
  elements (underlined for emphasis):

  bottom [10, 50, 19, 54, 30, 67, 67, 30, 54, 19, 50, 10] top

  Note that the mirrored version is added on to the top of what was
  originally in the stack. The bottom half of the stack contains the
  original numbers in the same order. If your method is passed an empty
  stack, the result should be an empty stack. If your method is passed a
  null stack, your method should throw an IllegalArgumentException.

  You may use one stack or one queue (but not both) as auxiliary storage
  to solve this problem. You may not use any other auxiliary data
  structures to solve this problem, although you can have as many simple
  variables as you like. You may not use recursion to solve this
  problem. For full credit your code must run in O(n) time where n is
  the number of elements of the original stack. Use the Queue interface
  and Stack/LinkedList classes from lecture.

I'm looking for ways to improve my future programming. Is this a good way to solve this problem? Currently, I'm surprised with the amount of work I'm doing with these Stacks and Queues data structures.

Yes, I know I'm being asked to use these data structures to solve the problem. But maybe there are ways that I need to think outside of the box to accomplish the same task?

I'm also looking for any bugs my code might have, so definitely let me know if you find any!

public static void mirror (Stack<Integer> s) {

    if (s == null) {
        throw new IllegalArgumentException();
    }

    Queue<Integer> q = new LinkedList<Integer>();

    while (!s.isEmpty()) {
        q.offer(s.pop());
    }

    while (!q.isEmpty()) {
        s.push(q.poll());
    }

    while (!s.isEmpty()) {
        q.offer(s.pop());
    }

    int size = q.size();
    for (int i = 0; i < size; i++) {
        int valq = q.poll();
        s.push(valq);
        q.offer(valq);
    }

    while (!s.isEmpty()) {
        q.offer(s.pop());
    }

    while (!q.isEmpty()) {
        s.push(q.poll());
    }
}

Overall this code looks decent. Just a few concerns:

For this kind of complex set of steps, it is always a good idea to add comments which will help the reader to understand the code
You can use the diamond operator here:

Queue<Integer> q = new LinkedList<>();

I might be opinionated, but I prefer the name val (or even v) rather than valq.

for (int i = 0; i < size; i++) {
    int val = q.poll();
    s.push(val);
    q.offer(val);
}


Correct control of execution of Java thread I'm writing an application that will use the JLayer library to play an mp3 file. Since the player does not have any methods to pause the playback - play() does not return until the song is finished - I decided to put it in a thread and simply control that thread. This is possible because one can execute play(1) which plays a single frame of the song. Putting this in a loop will thus play the entire song smoothly.

What I have done is the following:

Create a Thread class Mp3PlayerThread that can be used to play a song in a different thread.
Create a wrapping class Mp3Player that holds a thread object and controls the volatile flags inside the Thread.

I built the object Mp3Player such that it should be thread safe. I.e., pass an instance of Mp3Player around. To do this I made all the control methods synchronized.

I am wondering if this code can be made more solid without using volatile variables? I have always learned that you should not use the volatile keyword if possible.

Mp3PlayerThread

public class Mp3PlayerThread extends Thread
{
    public  volatile boolean playing = false;
    public  volatile boolean abort   = false;
    private Player   player;
    private String   path;

    public Mp3PlayerThread(String file)
    {
        this.path = file;
    }

    public void run()
    {
        // http://stackoverflow.com/questions/7313657/should-you-synchronize-the-run-method-why-or-why-not
        try
        {
            FileInputStream     fis = new FileInputStream(this.path);
            BufferedInputStream bis = new BufferedInputStream(fis);
            this.player             = new Player(bis);

            while (!abort)
            {
                if (playing)
                    player.play(1);
            }
            Printer.debugMessage(this.getClass(), "end of file or stopped");

        } catch (FileNotFoundException | JavaLayerException e)
        {
            e.printStackTrace();
        }
    }
}

Mp3Player

public class Mp3Player
{
    private Mp3PlayerThread playerThread;
    private String          song;

    public Mp3Player(String filename)
    {
        song = filename;
    }
    /**
     * Initializes the playerThread with the given song, sets the flag to true
     * and starts playing the song.
     */
    public synchronized void play()
    {
        playerThread         = new Mp3PlayerThread(song);
        playerThread.playing = true;
        playerThread.start();
    }
    /**
     * Pauses the execution of the playerThread.
     */
    public synchronized void pause()
    {
        Printer.debugMessage(this.getClass(), "paused");
        playerThread.playing = false;
    }
    public synchronized void resume()
    {
        Printer.debugMessage(this.getClass(), "resuming");
        playerThread.playing = true;
    }
    public synchronized void stop()
    {
        Printer.debugMessage(this.getClass(), "stopping");
        playerThread.abort = true;
        playerThread       = null; // Needed or not?
    }
}

Lets start with the instance variables.

public class Mp3PlayerThread extends Thread
{
    public  volatile boolean playing = false;
    public  volatile boolean abort   = false;
    private Player   player;
    private String   path;
    //...
}

You shouldn't be directly exposing playing and abort. Doing this hinders your ability to change the internal implementation of your class and remain backwards compatible.
Trying to line up you variable declarations will be a losing battle. It looks nice now, but when you add a new variable, you might have to completely change everything. This will make looking at diffs harder because some lines will be changed even though they are not directly involved in the new functionality. Additionally, an argument could be made that all of the variable names should be lined up instead of having player line up with boolean.
path is set once in the constructor and never changed. Setting it to be final could prevent you from accidentally changing it later with the expectation of it having an effect.

The URL at the beginning of the run method is not helpful. There is no context explaining why it is relevant to this piece of code. The post asks about something that this code is not doing. The general consensus of the post is "there is no reason to do that and I have never seen that done."

public void run() {
    FileInputStream     fis = new FileInputStream(this.path);
    BufferedInputStream bis = new BufferedInputStream(fis);
    this.player             = new Player(bis);
    //...
}

By writing your class this way, it is always forced to read a file from disk. What happens if you want to stream the song over a socket? Passing the input stream in to the constructor (or even the Player instance) would make your class more flexible and easier to test.

while (!abort)
{
    if (playing)
        player.play(1);
}

If playing is set to false, this thread will spin and waste cpu time.

There is no reason Mp3PlayerThread to be a subclass of Thread. Making it a simple class that implements Runnable would allow you to use it in more contexts and not incur the heavy costs of creating a thread every time an instance is instantiated. An example of the expanded context would be to create a thread pool of size one. Then you can schedule multiple songs to be played just by adding an instance to the execution pool for each new song.

public Mp3Player(String filename)
{
    song = filename;
}

public Mp3PlayerThread(String file)
{
    this.path = file;
}

Why are the variable names changing when they are all the same thing? Variable shadowing is not a valid excuse as you can use this. or prefix your member variables with an underscore.

In general, you should decide if you want to use this. or not and be consistent.


First time using Tkinter This is my first time using Tkinter and really trying to get use to using classes in Python. I wonder if I'm using self entirely too much, but here is my code:

from Tkinter import *

class MainFrame(object):
    def __init__(self):
        self.display()
    #display all the labels for the main frame
    def labels(self):
        #adding x-cord label
        x_cord_label = Label(self.top, text = 'X-Coordinate').grid(row=0,column = 0)
    #calls the visible features
    def display(self):
        self.top = Tk()
        self.labels()
        self.entries()
        self.buttons()
        #must remain at bottom or wont be displayed
        self.top.mainloop()

    #display all the entry boxes for the main frame
    def entries(self):
        x_cord_entry = Entry(self.top,bd=5).grid(row=0,column = 1)

    def buttons(self):
        x_cord_button = Button(self.top, text = 'Enter').grid(row=0,column = 2)

I am planning on main just having the mainframe being generated.

main.py
from mainFrame import MainFrame
def main():
    window = MainFrame()

if __name__=="__main__":
    main()

Python has a set of rules for how documentation strings should look like. These strings are called docstrings in Python and the corresponding PEP is PEP257. You basically just move your comments from above the function name inside and make them strings:

def display(self):
    """Calls the visible features."""
    self.top = Tk()
    self.labels()
    self.entries()
    self.buttons()
    # must remain at bottom or wont be displayed
    self.top.mainloop()

Python also has an official style-guide, PEP8, which recommends adding a single space after the # in a comment, as well as using a single blank line between class methods (and two between class or normal function definitions). It also recommends using a space after a comma in an argument list, but no spaces surrounding = for keyword arguments.

Your method display could be moved completely to the __init__, but it is also fine as a separate method, in case you ever want to call it again to reset your application / display it again after changing some values. I would swap the display and labels methods, though, it makes for easier reading.

Regarding your doubt if you are overusing self, i think you are rather underusing it than overusing. I'm not exactly sure how Tkinter handles objects, but in Python in general, objects can and will be garbage-collected once they go outside of scope. So, after e.g. the method labels ran, the local variable x_coord_label will be removed and the associated object deleted (unless it is still being referenced from somewhere else, like the Tk main loop for example). So I would store these labels/buttons etc also as class members. This is handy if you ever want to modify them as well (changing the text label on a button is a quite regular feature...).

In general, you should avoid using from X import *, because it clutters your global namespace and might lead to name conflicts. With Tkinter, many people use it anyway, mostly because one tends to use a lot of its functions, so selectively importing from TKinter import Button, Label, ... becomes too long and unreadable and always writing Tkinter.Button is also quite verbose. As a compromise you can use import Tkinter as tk, which is shorter and still has its own namespace.

Final code:

import Tkinter as tk

class MainFrame(object):

    def __init__(self):
        self.display()

    def display(self):
        """Calls the visible features."""
        self.top = tk.Tk()
        self.labels()
        self.entries()
        self.buttons()
        # must remain at bottom or wont be displayed
        self.top.mainloop()

    def labels(self):
        """Display all the labels for the main frame."""
        # adding x-cord label
        self.x_cord_label = tk.Label(
            self.top, text='X-Coordinate').grid(row=0, column=0)

    def entries(self):
        """Display all the entry boxes for the main frame."""
        self.x_cord_entry = tk.Entry(self.top, bd=5).grid(row=0, column=1)

    def buttons(self):
        self.x_cord_button = tk.Button(
            self.top, text='Enter').grid(row=0, column=2)

If you want to extend this further, you probably want to add more than one button. In this case, you will need to decide on a datastructure to hold your texts and buttons. So in the end it might look more like this:

class MainFrame(object):

    def __init__(self, button_texts):
        self.button_texts = button_texts
        self.top = tk.Tk()
        self.setup_rows()
        self.top.mainloop()

    def setup_rows(self):
        """
        Display all the rows with labels, entries and buttons
        for the main frame.
        """
        self.rows = []
        top = self.top
        for i, text in enumerate(self.button_texts):
            row = [tk.Label(top, text=text).grid(row=i, column=0),
                   tk.Entry(top, bd=5).grid(row=i, column=1),
                   tk.Button(top, text='Enter').grid(row=i, column=2)]
            self.rows.append(row)


Testing factory and beforemethod I'm trying to find out if it is possible to simplify my framework a little bit more. The current solution works, but it does cause some code duplication for me and some other issues that might come down the road.

public class Browsers {

  private String Browser;
  private String Windows;
  private String Ver;

  protected WebDriver driver;

  @BeforeTest
  public void BeforeMethod() {

    DriverFactory driverfactory = new DriverFactory();
    driver = driverfactory.createdriver(Browser);
  }

  @AfterTest
  public void AfterMethod() {}

  public Browsers(String Browser, String Windows, String Ver) {
    this.Browser = Browser;
    this.Windows = Windows;
    this.Ver = Ver;

  }

  @DataProvider
  public static Object[][] dataProviderMethod() {
    return new Object[][] {{"FF", "WIndows", "10"}, {"Chrome", "Windows", "10"}};
  }

}

public class DriverFactory {

  private WebDriver driver;

  public WebDriver createdriver(String Browser) {
    if (Browser.contains("FF")) {

      driver = new FirefoxDriver();
    }

    else {
      driver = new ChromeDriver();
    }
    return driver;
  }

}

public class LoginTests extends Browsers {

  @Factory(dataProvider = "dataProviderMethod")
  public LoginTests(String Browser, String Windows, String Ver) {
    super(Browser, Windows, Ver);
  }

  @Test
  public void LoginValid() {

  }

  @Test
  public void LoginInvalid() {

  }

}

Problem

Currently, I'm able to run tests when I create a factory on each of the test classes.

I have to write a constructor to all the test classes that I create. Which causes me some code duplication. When I have 100 tests then it will be quite a lot. Is it possible to avoid it.

First of all, calling your super class "Browsers" is wrong, because it doesn't have anything to do with browsers. It is probably a TestBase class, if your Test extends it.
You don't have to have duplicates and you don't have to have a constructor in your base class. Tests doesn't care about what browser it is executed on, so you should handle this information in TestBase class (currently Browsers). Personally I handle it in configuration/properties file. In that case I can switch easily and modify values from continuous integration server (i.e TeamCity).


Most common letter in a list I wrote this blog post, but received some feedback that there are better ways to handle the situation. I'd like to get some more opinions. Here is the topic:

  Given a list of letters, I want to find the most commonly seen one.

# create the list of letters
my_list = ['b', 'b', 'b', 'c', 'a', 'b']

# simple sentence to make all more "user friendly"
sentence = '\nLetter which repeat the most is: '

# 3 lists declared globally which will store our values
a_list = []
b_list = []
c_list = []

# for loop which control the letters in the list and append them to the respective list
for v in my_list:
    if v == 'a':
        a_list.append(v)

    elif v == 'b':
        b_list.append(v)

    elif v == 'c':
        c_list.append(v)

# create a variable which store the name of the letter and how many times appear in the main list
A = a_list[0], len(a_list)
B = b_list[0], len(b_list)
C = c_list[0], len(c_list)

# variable which store length values
total_list = A[1], B[1], C[1]

# find the max value
largest = max(total_list)

# in this loop we are going to compare each value from total_list to the largest one
for i, val in enumerate(total_list):
    if total_list[i] == largest:
        x = total_list[i]  # assign to "x" this value

        # do a check for all the values
        # if the condition is satisfied, then print the sentence and letter (to uppercase)
        if x == A[1]:
            print(sentence + str(A[0]).upper())
        elif x == B[1]:
            print(sentence + str(B[0]).upper())
        elif x == C[1]:
            print(sentence + str(C[0]).upper())

I've heard that collections.Counter could be used for this, but I'm not sure that the output will be the same. I might be able to use it to replace some of my code above, but will I end up with the same result?

There are a few problems with this; mainly that you only support the letters a, b, and c. It isn't necessarily a problem if those are the only letters you'll ever get, but what is a problem is that you're making identical variables, with identical behavior, for each letter. Then for every letter you want to add in the future, you have to repeat yourself. Anytime you see yourself with a pattern like this, it is screaming for you to use a dictionary, or situationally a list of lists.

Making that change, we'd end up with something like this

counter = {}
for letter in my_list:
    if letter not in counter:
        counter[letter] = 0
    counter[letter] += 1

largest = 0
best_letters = []
for letter, count in counter.items():
    if count > largest:
        largest = count
        best_letter = [letter]
    elif count == largest:
        best_letter.append(letter)

print()
for letter in best_letters:
    print("Letter which repeats the most is: {}".format(letter.upper()))

Now we don't need the separate logic for each possible letter; we also don't need that hacky loop where you find the largest and then loop through.

We can make this better though, as you implied with collections.Counter. A Counter already does a lot of this work. For example, to get the count we just do

counter = Counter(my_list)

Then to get the best one, we just do

best_letter, largest = counter.most_common(1)[0]

This makes your total program look like this

best_letter, _ = Counter(my_list).most_common(1)[0]
print()
print("Letter which repeats the most is: {}".format(best_letter.upper()))

In general, it is always good to write functions to encapsulate behavior, but in this case you can probably just inline the use of the Counter.

Now if we want to get all of the letters that are most frequent, we can change it pretty easily. We'll do this instead:

most_common = counter.most_common(len(counter))
largest = most_common[0][1]

for letter, count in most_common:
    if count < largest:
        break

    print("Letter which repeats the most is: {}".format(letter.upper())

That'll grab all of values in the order of most to least frequent, and then you loop through all of the ones that are most commonly seen. Note that there is no implicit sorting within the ones that share a frequency; if you want further constraints on that it'll take additional logic.


MD5 shuffling with a defined pattern of numbers I've created a MD5 shuffler with a defined number pattern.  Does this make sense?  Will this make storing passwords more secure?  Is it efficient?

    <?php
echo "
";
$md5 = "e2fc714c4727ee9395f324cd2e7f331f";
echo "Old hash: " . $md5 . "
";
$hash = str_split($md5, 1);
$shuffle = explode(",", "24,29,21,23,2,30,10,22,6,28,26,11,8,19,9,20,16,3,0,14,18,15,12,25,5,4,31,1,7,27,13,17");
$newhash = "";
for ($i = 0; $i < 32; $i++) {
$s = $shuffle[$i];
$newhash .= $hash[$s];
}
echo "New Hash: " . $newhash . "
";
$newhash = str_split($newhash, 1);
$reverse = "";
for ($i = 0; $i < 32; $i++) {
$s = $shuffle[$i];
$reverse[$s]= $newhash[$i];
}
ksort($reverse);
$reversehash = "";
foreach ($reverse as $value) {
$reversehash .= $value;
}
echo"Reversed hash: " . $reversehash . "
";
echo "
";
?>

Will this make storing passwords more secure?

Using MD5 for passwords is a bad idea, not because of its cryptographic weaknesses, but because it's fast. This means that an attacker can try billions of candidate passwords per second on a single GPU.

I wouldn't even bother creating my own secure hashing functions if I'm not an expert at it. There are libraries out from people who have already done all the hard work for you and have tested them thoroughly for security. PHPass is one of them.

Matter of fact, if you run PHP 5.5 or greater, you can use its native function: password_hash().


Bresenham Line Drawing Algorithm int Octet(double m, int Ax, int Ay, int Bx, int By)
{
    int octetNo = -1;
    double dx = Bx - Ax;
    double dy = By - Ay;

    //m = dy / dx;

    if(0<=m && m<=1)
    {
        if(Ax<Bx) octetNo = 1;
        else if(Bx<Ax) octetNo = 5;
    }
    else if(0>=m && m>=-1)
    {
        if(Bx<Ax)
            octetNo = 4;
        else if(Ax<Bx) octetNo = 8;
    }
    else if(m>1)
    {
        if(Ay<By) octetNo = 2;
        else if(By<Ay) octetNo = 6;
    }
    else if(-1>m)
    {
        if(Ay<By) octetNo = 3;
        else if(By<Ay) octetNo = 7;
    }

    return octetNo;
}

int NextDiIfDiIsGreaterThanOrEqualtoZero(double m, int di, int x1, int y1, int x2, int y2)
{
    int di_plus_1 = -1;
    int dx = x2 - x1;
    int dy = y2 - y1;

    int octet = Octet(m, x1, y1, x2, y2);

    switch(octet)
    {
    case 1:
    case 5:
        di_plus_1 = di - 2 * (dx - dy);
        break;
    case 2:
    case 6:
        di_plus_1 = di + 2 * (dx - dy);
        break;
    case 3:
    case 7:
        di_plus_1 = di - 2 * (dx + dy);
        break;
    case 4:
    case 8:
        di_plus_1 = di + 2 * (dx + dy);
        break;
    }

    return di_plus_1;
}

int NextDiIfDiIsSmallerThanZero(double m, int di, int x1, int y1, int x2, int y2)
{
    int di_plus_1 = -1;
    int dx = x2 - x1;
    int dy = y2 - y1;

    int octet = Octet(m, x1, y1, x2, y2);

    switch(octet)
    {
    case 1:
    case 5:
        di_plus_1 = di - 2 * dy;
        break;
    case 2:
    case 6:
        di_plus_1 = di + 2 * dx;
        break;
    case 3:
    case 7:
        di_plus_1 = di - 2 * dx;
        break;
    case 4:
    case 8:
        di_plus_1 = di + 2 * dy;
        break;
    }

    return di_plus_1;
}

int InitialValueOfDi(double m, int x1, int y1, int x2, int y2)
{
    int init_di = -1;
    int dx = x2 - x1;
    int dy = y2 - y1;

    int octet = Octet(m, x1, y1, x2, y2);

    switch(octet)
    {
    case 1:
    case 5:
        init_di = dx - 2 * dy;
        break;
    case 2:
    case 6:
        init_di = 2 * dx - dy;
        break;
    case 3:
    case 7:
        init_di = (-2) * dx - dy;
        break;
    case 4:
    case 8:
        init_di = 2 * dy + dx;
        break;
    }

    return init_di;
}

void BresLine(int x1, int y1, int x2, int y2, int color)
{
    int dy = y2 - y1;
    int dx = x2 - x1;
    double m = (double)dy/(double)dx;
    int di = InitialValueOfDi(m, x1, y1, x2, y2);
    int limit = ((abs(dx)>abs(dy))?abs(dx):abs(dy))/2;
    int x = x1;
    int y = y1;

    for(int i=0 ; i<=limit; i++)
    {
        PlotPixel(x, y, color);

        if(di>=0)
        {
            di = NextDiIfDiIsGreaterThanOrEqualtoZero(m, di, x1, y1, x2, y2);
            y++;
        }
        else
        {
            di = NextDiIfDiIsSmallerThanZero(m, di, x1, y1, x2, y2);
        }
        x++;
    }
}

Disclaimer: I know nothing of C++, but I'll help you as much as I can.

Overall, your code looks good. A+ for readability! Probably a missing spot somewhere, but no biggy. Still, your code is easy to read and follow.

On your BresLine(), you have this:

int dy = y2 - y1;
int dx = x2 - x1;
double m = (double)dy/(double)dx;
[...]
int limit = ((abs(dx)>abs(dy))?abs(dx):abs(dy))/2;

You could try to store that abs() somewhere, to shave on re-calling it.

You have the functions NextDiIfDiIsGreaterThanOrEqualtoZero() and NextDiIfDiIsSmallerThanZero(). They have giant names. And the code is almost the same. I suggest the following:

int NextDi(double m, int di, int x1, int y1, int x2, int y2)
{
    int di_plus_1 = -1;
    int dx = x2 - x1;
    int dy = y2 - y1;

    int octet = Octet(m, x1, y1, x2, y2);

    switch(octet)
    {
        case 1:
        case 5:
            di_plus_1 = di - 2 * (di > 0 ? dx - dy : dy);
            break;
        case 2:
        case 6:
            di_plus_1 = di + 2 * (di > 0 ? dx - dy : dx);
            break;
        case 3:
        case 7:
            di_plus_1 = di - 2 * (di > 0 ? dx - dy : dx);
            break;
        case 4:
        case 8:
            di_plus_1 = di + 2 * (di > 0 ? dx - dy : dy);
            break;
    }

    return di_plus_1;
}

It isn't the prettiest thing on Earth, but may work for you. To call it, you don't have to change anything! Just remove that if on BresLine. With some bitwise operations, you can cut down this code by a lot!

If you see something innacurate, please tell me. I haven't touched on the remaining functions, but I'm specially worried about the Octet function.


Reversing the lines of file I have written a program to reverse the lines of a file. I made use of the String [] approach with the String.split(separator) using a custom separator.

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.OutputStreamWriter;

public class ReverseTheLinesOfaFile {

private static final String  SEPERATOR="-";

/**
 * This function reads the data of the file and then reverses the lines of the file keeping
 * the content as the same
 * @param fileName
 */
private void reverseLines(String fileName) {
    // TODO Auto-generated method stub
    File file = new File(fileName);
    //get the readers ready
    FileInputStream inputFile = null;
    StringBuilder textInput= new StringBuilder();
    String line=null;
    try {

        inputFile = new FileInputStream(file);
        BufferedReader reader = new BufferedReader(new InputStreamReader(inputFile));

        //read the file line by line and append the separator
        while ((line= reader.readLine())!=null){
            textInput.append(line+SEPERATOR);
        }
        reader.close();
        inputFile.close();

        //get the writers ready
        FileOutputStream outputFile = new FileOutputStream(file);
        BufferedWriter writer = new BufferedWriter(new OutputStreamWriter(outputFile));

        String[] lines   = textInput.toString().split(SEPERATOR);

        //write line by line to the file
        for (int i= lines.length-1 ; i > -1 ; --i){
            writer.write(lines[i]+"\n");
        }
        writer.flush();
        writer.close();
        System.out.println("file reversing done");
    } catch (FileNotFoundException e) {
        System.out.println("The file with name ="+file.getName()+" could not be found");
        e.printStackTrace();
    } catch (IOException e) {
        System.out.println("There has been a IO exception");
        e.printStackTrace();
    }
}

public static void main(String[] args) {

    ReverseTheLinesOfaFile linesOfaFile = new ReverseTheLinesOfaFile();
    linesOfaFile.reverseLines("data.txt");
}

}

I want to know whether this can be done even more efficiently and without the use of the Seprator variable. The file can contain all sorts of characters in the ASCII table.

You have a number of open resoruces that you do not close properly, FileOutputStreams, BufferedWriters, etc. These should be handled with a try-with-resources block.

Additionally, the standard Java libraries contain helper methods that will make this operation almost trivial... See Files.readAllLines()

Together, this could all be done as:

private static void reverseLines(String fileName) {
    Path path = Paths.get(fileName);
    try {
        List<String> lines = Files.readAllLines(path);
        try (BufferedWriter writer = Files.newBufferedWriter(path)) {
            ListIterator<String> lineIterator = lines.listIterator(lines.size());
            while (lineIterator.hasPrevious()) {
                writer.write(lineIterator.previous());
                writer.newLine();
            }
        }
    } catch (IOException ioe) {
        ioe.printStackTrace();
    }

}


Optimization of Barnes-Hut Multithread Insertion algorithm I'm currently working on optimizing a Barnes-Hut implementation (connected to a previous post I did) and I need help to optimize it further.

After some testing, the main problem appears to be in the Insertion algorithm, which takes roughly 3-10 times as long as the second longest part of the program.

The basis of the program are a number of threads that each act as a worker that does the following when building the tree:

//particle[] is a list that is set in the constructor
public void run(){
    boolean run = true;

    while(run){
        i = barnesHut.getIndex();

        if(i < particle.length){ tree.insertThreaded(particle[i], thread); }
        else{ run = false; }
    }
}

the getIndex function can be found in the BarnesHut class which handles the start of the program and the insertion algorithm. It's synchronized to work as a

public synchronized int getIndex(){
    return index++;
}

Now, the insertion algorithm looks like this:

//entryLock and quadLock are two atomic mutex locks
//childs[][] are the leaves to the tree structur that makes up a Barnes-Hut

public void insertThreaded(BHBody body, int threadID){
    entryLock.lock();
    numberOfParticles++;

    if(numberOfParticles == 1){
        existingParticle = body;
        entryLock.unlock();
    }
    else if(numberOfParticles == 2){
        quadLock.lock();

        entryLock.unlock();
        int[] position = getQuadForBody(existingParticle);
        if(childs[position[0]][position[1]] == null){
            startQuad(position[0],position[1]);
        }
        quadLock.unlock();

        childs[ position[0] ][ position[1] ].insertThreaded(existingParticle, threadID);

        quadLock.lock();
        position = getQuadForBody(body);
        if(childs[position[0]][position[1]] == null){
            startQuad(position[0],position[1]);
        }
        quadLock.unlock();

        childs[ position[0] ][ position[1] ].insertThreaded(body, threadID);
    }
    else if(numberOfParticles > 2){
        entryLock.unlock();

        quadLock.lock();
        int[] position = getQuadForBody(body);
        if(childs[position[0]][position[1]] == null){
            startQuad(position[0],position[1]);
        }
        quadLock.unlock();

        childs[position[0]][position[1]].insertThreaded(body, threadID);
    }
}

The locks are to make sure that no other thread gets into the way of the thread when it's creating something vital, such as a new leaf or even trying to update the number of particles.

The startQuad() function which creates a new leaf for the current node:

public void startQuad(int x, int y){
    if(x == 0 && y == 0)
        childs[0][0] = new BHnode(xStart,xMid,yMid,yEnd, debug, range,correction);
    if(x == 0 && y == 1)
        childs[0][1] = new BHnode(xStart,xMid,yStart,yMid, debug, range,correction);
    if(x == 1 && y == 0)
        childs[1][0] = new BHnode(xMid,xEnd, yMid,yEnd, debug, range,correction);
    if(x == 1 && y == 1)
        childs[1][1] = new BHnode(xMid,xEnd,yStart,yMid, debug, range,correction);
}

The getQuad() function which returns the quad for the body to be inserted into:

public int[] getQuadForBody(BHBody body){
    int x = 0, y = 0;
    if( body.getX() > xMid ){
        x++;
    }
    if( body.getY() < yMid ){
        y++;
    }
    int[] a = {x,y};
    if(debug){
        System.out.println("setting ("+body.getX()+","+body.getY()+") at ("+x+","+y+") with mid " +xMid+","+yMid);
        helper.sleepSeconds(2);
    }
    return a;
}

If anyone has any suggestions on how to improve this code I would be grateful. I've tried a lot of different things, from completly rewriting parts of the code to making things in a different order.

You should know, what's the bottleneck. Do you have two locks altogether? This doesn't sound good, as each part of the tree can be worked independently on. This means you could run on hundreds of CPUs efficiently.

The operations you do under lock are rather simple, which means that the overhead of locking can be significant. Can't you separate the three into a few parts and assign one CPU to each part?

Now some boring general comments:

boolean run = true;
while(run){
    i = barnesHut.getIndex();
    if(i < particle.length){ tree.insertThreaded(particle[i], thread); }
    else{ run = false; }
}

Where is i defined? Anyway, unobfuscate to

while (true) {
    int i = barnesHut.getIndex();
    if (i >= particle.length) break;
    tree.insertThreaded(particle[i], thread);
}

public synchronized int getIndex(){
    return index++;
}

That's what AtomicInt#incrementAndGet() is for.

Use better spacing, if (i < particle.length) { instead of if(i < particle.length){.


Cycling between element above and below current array element The title sucks, but it's really very simple - given an array and a starting element, cycle between one element above, below, then two element above, and below the starting element. For example:

Start: 5
5 > 6 > 4 > 7 > 3 > 8 > 2 ...

It doesn't matter if the code initially start with the element above or below. To complicate things a little, when you reach the end or beginning of the array, the code should continue to pick valid array elements, instead of going into invalid positions:

Start: 2
2 > 3 > 1 > 4 > 0 > 5 > 6 > 7 ...

This is the code I have. It works, but I don't really like the way to fulfil the second condition above, having that two lines repeated twice.

var distance = 0;

while ((currentHeight + this.heights[index]) < height) {
    sections.push(index);
    currentHeight += this.heights[index];

    distance = -distance + (distance >= 0 ? -1 : 1);
    index += distance;

    if (index < 0 || index >= this.heights.length) {
        distance = -distance + (distance >= 0 ? -1 : 1);
        index += distance;
    }
}

distance is the distance from the starting point, index holds the current position. The code cycles with these two lines:

distance = -distance + (distance >= 0 ? -1 : 1);
index += distance;

and it resolves the invalid array position problem by simply rerunning those two lines if it finds an invalid array position.

First off, your code was a bit difficult to read, because you combined the algorithm you need with your functionality, that you didn't explain. It's not only easier for people helping you, but also yourself, if you try to separate your problem from the application.

Your attempt was to calculate an index from the previous index. I instead created a function, that mapped a increasing index (0, 1, 2, 3, 4,...) to the targets (0, 1, -1, 2, -2,...). So we are looking for a function f(i) where f(0) = 0, f(1) = 1, f(2) = -1, f(3) = 2, etc.

I came up with: (i % 2 ? 1 : -1) * Math.floor( (i+1) / 2 ).

Now you just need a condition to skip new indexes outside your array. I came up with the following:

var yourArray = new Array(10);
var start = 2;

var i = 0;
var max = yourArray.length;

while (max) {
  var newI = (i%2?1:-1) * Math.floor((i+1)/2) + start;
  if (newI >= 0 && newI < yourArray.length) {
    document.write( newI + "<br>");
    // do something with yourArray[newI];
    max--;
  }
  i++;
}

My problem was how to decide when to stop the loop. For now i just loop over twice the length of the array. That will usually lead to some unnecessary iterations, but maybe someone can come up with a better solution, or there maybe a condition that comes from your application. UPDATE: Found a solution.


How would you ruby-fy this non-AR model class for Ruby on Rails app? This is a class from my rails application. It is used as non-database model in my RoR application and uses syntax similar to AR design pattern. I wonder is there a way to make this look more like Ruby code?

class Role

  def self.all
    role_list = Array.new

    r1 = Role.new
    r1.id = 1
    r1.symbol = :owner
    r1.name = "Owner"
    r1.has_all_permissions = true
    role_list << r1

    r2 = Role.new
    r2.id = 2
    r2.symbol = :admin
    r2.name = "Administrator"
    r2.has_all_permissions = false
    role_list << r2

    r3 = Role.new
    r3.id = 3
    r3.symbol = :gamemaster
    r3.name = "Gamemaster"
    r3.has_all_permissions = false
    role_list << r3

    role_list
  end

  def self.find(id)
    roles = Role.all

    roles.each do |role|
      if role.id == id
        return role
      end
    end

    nil
  end

  def self.find_by_symbol(symbol)
    roles = Role.all

    roles.each do |role|
      if role.symbol == symbol
        return role
      end
    end

    nil
  end

  def assign_permission(symbol)
    permission = Permission.find_by_symbol(symbol)

    if permission == nil
      return nil
    end

    permission_roles = PermissionRole.where(:permission_id => permission.id, :role_id => self.id)

    if permission_roles.size == 0
      permission_role = PermissionRole.new
      permission_role.permission_id = permission.id
      permission_role.role_id = self.id
      return permission_role.save
    end

    true
  end

  def has_permission?(symbol)
    permission = Permission.find_by_symbol(symbol)

    if permission == nil
      raise StandardError, "Permission does not exist"
    end

    permission_roles = PermissionRole.where(:permission_id => permission.id, :role_id => self.id)

    if permission_roles.size > 0
      return true
    else
      return false
    end
  end

  attr_accessor :id
  attr_accessor :symbol
  attr_accessor :name
  attr_accessor :has_all_permissions

end

For starters:

def initialize(attributes = {})
  attributes.each do |attr, value|
    self.send "#{attr}=", value
  end
end

Would allow you to turn:

r1 = Role.new
r1.id = 1
r1.symbol = :owner
r1.name = "Owner"
r1.has_all_permissions = true
role_list << r1

into:

role_list << Role.new {:id => 1,
                       :symbol => :owner,
                       :name => "Owner",
                       :has_all_permissions => true}

which is much more ActiveRecord-like.

Storing the roles_list in a global variable might be acceptable for your application and would also allow you to have:

def self.create(attributes = {})
  $roles_list << Role.new(attributes)
end

and:

def self.all
  return $roles_list if $roles_list

  # rest of code to init $roles_list with default roles
end

for further ActriveRecord-ness

Then:

def self.find(id)
  roles = Role.all

  roles.each do |role|
    if role.id == id
      return role
    end
  end

  nil
end

could be re-written like so:

def self.find(id)
  Role.all.find{|r| r.id == id}
end

That is because Array has its own #find method. The same refactoring could be applied to self.find_by_symbol.

As a final note, I would move the attr_accessor lines to the top.

Hope I helped :-)


Partial octree implementation I am just now learning Scala a bit on my own time.  I wrote some code that works, but was wondering if you could eyeball it to see if its structure can be improved.  It is a partial octree implementation.

class octree(min_x:Double,
            min_y:Double,
            min_z:Double,
            max_x:Double,
            max_y:Double,
            max_z:Double
       ){
            var children:Array[octree] = new Array[octree](8)
            val min = (min_x,min_y,min_z)
            val max = (max_x,max_y,max_z)

            def side(x:Double,y:Double):Int={
                if(x<y) 1
               else 0
           }
           def index(x:Double,y:Double,z:Double):Int={
                 val center=(0.5*(max._1+min._1),0.5*(max._2+min._2),
                    0.5*(max._3+min._3))
                (side(center._1,x))|(side(center._2,y)<<1)|     (side(center._3,z)<<2)
          }
          def add(x:Double,y:Double,z:Double):octree={
                var i=index(x,y,z)
                print(" "+i)
                if(children(i)==null){
                    var min2_x=min._1
                    var min2_y=min._2
                    var min2_z=min._3
                    var max2_x=max._1
                    var max2_y=max._2
                    var max2_z=max._3

                    val center=(0.5*(max._1+min._1),0.5*(max._2+min._2),
                    0.5*(max._3+min._3))

                    if((i&1)==0) max2_x=center._1
                    else min2_x=center._1
                    if((i&2)==0) max2_y=center._2
                    else min2_y=center._2
                    if((i&4)==0) max2_z=center._3
                    else min2_z=center._3

                    children(i) = new        octree(min2_x,min2_y,min2_z,max2_x,max2_y,max2_z)
                    children(i)
            }
            else children(i).add(x,y,z)
    }
    def find(x:Double,y:Double,z:Double):octree={
             var i=index(x,y,z)
            print(" "+i)
             if(children(i)==null){
                    this
             }
            else children(i).find(x,y,z)
    }
 }

Some ideas:

Properly indent your code and strictly adhere to some style guide. (For example it's very uncommon to have class name starting with a lowercase letter.) I recommend you to read some style guide or study the code of some publicly available libraries. This is very important for readability and future maintainability of your code. It doesn't matter if you will be reading it or somebody else. Code is written once but read many times.
Define a custom class for vectors. (Having a function with 6 arguments is inconvenient and error prone and it is a signal for looking for alternative solutions).
Use this class to implement vector operations on it.
Variables that aren't modified should be declared val.
side could be made local to the index method.
center can be precomputed and reused.
Instead of modifying variables when computing adding a node, it's often more idiomatic to describe the new cell declaratively. Using a helper function can shorten the code.
Optimize recursive functions using tailrec, if possible.

After some refactoring:

case class Vector(x: Double, y: Double, z: Double) {
  def +(that: Vector) = Vector(this.x + that.x,
                               this.y + that.y,
                               this.z + that.z);
  def *(scalar: Double) = Vector(this.x * scalar,
                                 this.y * scalar,
                                 this.z * scalar);
}

class Octree(min: Vector, max: Vector) {
  val children: Array[Octree] = new Array[Octree](8)
  val center = (max + min) * 0.5

  def index(p: Vector): Int = {
    @inline
    def side(x: Double, y: Double): Int =
      if (x < y) 1
      else 0
    (side(center.x, p.x)) |
    (side(center.y, p.y)<<1) |
    (side(center.z, p.z)<<2)
  }

  @annotation.tailrec
  final def add(point: Vector): Octree = {
    var i = index(point)
    debug(i)
    if (children(i) == null) {
      @inline
      def pick(b: Int, f: Vector => Double, l: Vector, r: Vector): Double =
        if (b == 0) f(l)
        else f(r)
      def split(v1: Vector, v2: Vector): Vector =
        Vector(pick(i & 1, _.x, v1, v2),
               pick(i & 2, _.y, v1, v2),
               pick(i & 4, _.z, v1, v2));
      children(i) = new Octree(
        split(center, min),
        split(max, center));
      children(i)
    } else
      children(i).add(point)
  }

  @annotation.tailrec
  final def find(p: Vector): Octree = {
    var i = index(p)
    debug(i)
    if(children(i) == null)
      this
    else
      children(i).find(p)
  }

  private def debug(message: => Any) {
    // if you comment out the print statement, `message`
    // won't be evaluated, so even if it's computationally intensive,
    // you don't need to comment out `debug` from the code.
    print(message.toString)
  }
}


Relying on short-circuit evaluation instead of using the IF control structure Examples of where I've started migrating to short-circuit evaluation:

PHP

$id=0;//initialized in case of no result
$r=mysql_query(SQL);
if($r && mysql_num_rows($r)>0){
  list($id)=mysql_fetch_row($r);
}

becomes

$id=0;
$r=mysql_query(SQL);
$r && mysql_num_rows($r)>0 && list($id)=mysql_fetch_row($r);

JavaScript

var Req=jQuery.ajax(OBJECT);
//Something has happened and now i want to abort if possible
if(Req && Req.abort){
  Req.abort();
}

becomes

var Req=jQuery.ajax(OBJECT);
//Something has happened and now i want to abort if possible
Req && Req.abort && Req.abort();

I'm familiar with the short if style of

if(Req && Req.abort) Req.abort();

but it feels clunkier than short-circuit evaluation.

This is an abuse of short-circuit evaluation.  It's not immediately obvious what the intent is.  Sure, i can eventually see that you're getting an ID, calling Req.abort(), whatever.  But i shouldn't have to decipher a line of code first to figure out what it does.
Conciseness never trumps readability in source code.  (Sure, minifying/packing JS has its benefits.  But you don't write packed/minified code; you start out with decent code and then run a tool on it.)

Since your intent is to do the one thing if the other stuff is true, the code should say that.


Simple bash script creates apache2 virtualhost config for subdomain Simple bash script to create apache2 virtualhost for localhost. Can be used for public subdomains on developer server with changes - "replace .localhost".

Tested on Ubuntu, but should work where dependency met.

Not for production, for development purposes only.

Use zenity for gui and pkexec for root permissions, so can be run without terminal, however some terminal output persist.

/etc/hosts file looks like: (so allowing add any subdomain)

127.0.0.1 localhost *.localhost

Note the wildcard, script doesn't create domains.

I'm not very experienced at bash scripting. Review and improve security, compatibility, maybe also some features with domains.

It's also on GitHub:(Updated) https://github.com/LeonidMew/CreateVirtualHost

#!/bin/bash

WEBROOT="/home/leonid/Web/" # root folder where subfolders for virtualhosts created
APACHEHOST="/etc/apache2/sites-available/050-" # prefix for virtualhost config file
A2ENSITE="050-" # short prefix for virtualhost config file
TMPHOST="/tmp/a2host-" # tmp prefix for virtualhost config while editing or rejecting

if ((`which zenity|wc -w` == 0)) # check dependency
then
    echo "Error: zenity not installed."
    exit
fi

if [ "$USER" == "root" ]
then
    zenity --error --text="You should not run this script as root but as user going to edit web files."
    exit
fi

HOST=`zenity --forms --add-entry=Name --text='Create virtualhost (= Folder name,case sensitive)'`
words=$( wc -w <<<"$HOST" )

if (($words == "0" || $words > 1)) # this not check for fully qualified sub domain name. ".localhost" added
then
    zenity --error --text="More then one word for sub domain or empty"
    exit
fi

HOSTFILE="$APACHEHOST$HOST"
HOSTFILE=$HOSTFILE".conf"   # apache virtualhost config file
DIR="$WEBROOT$HOST"         # folder used as document root for virtualhost

# virtualhost template
cat >$TMPHOST$HOST <<EOF
<VirtualHost *:80>
    ServerAdmin webmaster@localhost
    DocumentRoot $DIR
    ServerName  $HOST.localhost
    ServerAlias $HOST.localhost
    <Directory "$DIR">
        AllowOverride All
        Require local
    </Directory>
    # Available loglevels: trace8, ..., trace1, debug, info, notice, warn,
    # error, crit, alert, emerg.
    LogLevel warn
</VirtualHost>
# vim: syntax=apache ts=4 sw=4 sts=4 sr noet
EOF

# edit virtualhost config
TEXT=`zenity --text-info --filename=$TMPHOST$HOST --editable`
words=$( wc -w <<<"$TEXT" )
if (($words == 0))
then
    echo "Cancel"
    rm $TMPHOST$HOST
    exit
fi
echo "$TEXT" > $TMPHOST$HOST

A2ENSITE=$A2A2ENSITE$HOST".conf" # params for a2ensite

echo "execute root tools with pkexec to create virtualhost"
[ -d "$DIR" ] || mkdir -p "$DIR"
pkexec /bin/bash <<EOF
chgrp www-data "$DIR"
chmod u=rwX,g=rX,o= "$DIR"
mv $TMPHOST$HOST $HOSTFILE
chown root:root $HOSTFILE
chmod u=rw,g=r,o=r $HOSTFILE
a2ensite $A2ENSITE
EOF

Notes:

Quote your variables. Ref Security implications of forgetting to quote a variable in bash/POSIX shells
Don't use ALLCAPS varnames. It's too easy to overwrite important shell variables like PATH.
A2ENSITE=$A2A2ENSITE$HOST".conf" -- I don't see the A2A2ENSITE variable anywhere

this is a perhaps a corollary of the ALLCAPS vars problem: they can be hard to read.

For Command Substitution, don't use backticks, use $( ... ). That syntax is (IMO) easier to read, and there are other advantages, such as nestability.
if ((`which zenity|wc -w` == 0)) -- use the bash builtin type command to see if there is a zenity command available: type -p zenity will return an unsuccessful exit status if there's no zenity in your path:

if ! type -p zenity >/dev/null

Although I don't really see the need for zenity. It would be super frustrating for the user who doesn't have it, being prevented from using your script. And the technical user who would be comfortable installing it is the type of user who doesn't need the bells and whistles, IMO.
To check if a string is empty, you don't need to call out to wc.

Not this:

TEXT=`zenity --text-info --filename=$TMPHOST$HOST --editable`
words=$( wc -w <<<"$TEXT" )
if (($words == 0))

but this:

text=$(zenity --text-info --filename="$filename" --editable)
if [ -z "$text" ]       # cancel if empty

use mktemp for temp files

tmphost=$(mktemp)

And just use "$tmphost" instead of $TMPHOST$HOST
you can tell bash to automatically delete the temp file when it exits:

trap "rm $tmphost" EXIT

validate user input for host: a case statement might make sense here:

host=$(zenity --forms --add-entry=Name --text='Create virtualhost (= Folder name,case sensitive)')
case "$host" in
    "")            zenity --error --text="Bad input: empty";      exit 1 ;;
    *"*"*)         zenity --error --text="Bad input: wildcard";   exit 1 ;;
    *[[:space:]]*) zenity --error --text="Bad input: whitespace"; exit 1 ;;
esac

I applaud your use of here-documents
use if [ "$(id -un)" = "root" ] instead of the USER variable.
mkdir -p silently does nothing if the directory already exists, so you don't need to test -d

Perhaps you want this:

#!/bin/bash

webroot="/home/leonid/Web/" # root folder where subfolders for virtualhosts created
apachehost="/etc/apache2/sites-available/050-" # prefix for virtualhost config file
a2ensite="050-"             # short prefix for virtualhost config file
tmphost=$(mktemp)
trap "rm $tmphost" EXIT

if [ "$USER" == "root" ]
then
    echo "You should not run this script as root but as user going to edit web files." >&2
    exit 1
fi

read -p"Create virtualhost (= Folder name,case sensitive)" -r host
case "$host" in
    "")            echo "Bad input: empty" >&2;      exit 1 ;;
    *"*"*)         echo "Bad input: wildcard" >&2;   exit 1 ;;
    *[[:space:]]*) echo "Bad input: whitespace" >&2; exit 1 ;;
esac

# braces only for readability
hostfile="${apachehost}${host}.conf"    # apache virtualhost config file
dir="${webroot}${host}"                 # folder used as document root for virtualhost

# virtualhost template
cat >"$tmphost" <<EOF
<VirtualHost *:80>
    ServerAdmin webmaster@localhost
    DocumentRoot $dir
    ServerName  $host.localhost
    ServerAlias $host.localhost
    <Directory "$dir">
        AllowOverride All
        Require local
    </Directory>
    # Available loglevels: trace8, ..., trace1, debug, info, notice, warn,
    # error, crit, alert, emerg.
    LogLevel warn
</VirtualHost>
# vim: syntax=apache ts=4 sw=4 sts=4 sr noet
EOF

# edit virtualhost config
editor=${VISUAL:-$EDITOR}
if [ -z "$editor" ]
then
    echo "edit '$tmphost' to your liking, then hit Enter"
    read -p "I'll wait ... "
else
    "$editor" "$tmphost"
fi
# probably want some validating here that the user has not broken the config

echo "execute root tools with pkexec to create virtualhost"
mkdir -p "$dir"

pkexec /bin/bash <<EOF
chgrp www-data "$dir"
chmod u=rwX,g=rX,o= "$dir"
mv "$tmphost" "$hostfile"
chown root:root "$hostfile"
chmod u=rw,g=r,o=r "$hostfile"
a2ensite "${a2ensite}${host}.conf"
EOF

Responding to your questions:

"determine if running in a terminal?" Yes with this obscure test:

if [ -t 0 ]; then echo "in a terminal"; fi

That tests file descriptor 0, which is stdin. If you're launching your script as a GUI, that test should be false.
editor=${VISUAL:-$EDITOR} sets the editor variable to the user's $VISUAL variable, or if that's not set, to the $EDITOR variable. Many programs use this to determine the user's preferred "terminal" editor. vim and emacs are two common values there. If neither of those are set, then the user gets to go edit that however he chooses.
"is read terminal only?" Yes

If you're going to aim for GUI and text versions, I'd use one script, but make sure all the common code is put into functions so you don't have to duplicate your code. For example:

get_virtual_host() {
    if [ -t 0 ]; then
        read -p "Create virtualhost (= Folder name,case sensitive)" -r host
    else
        host=$(zenity --forms --add-entry=Name --text='Create virtualhost (= Folder name,case sensitive)')
    fi
    case "$host" in
        "")            echo "Bad input: empty" >&2;      exit 1 ;;
        *"*"*)         echo "Bad input: wildcard" >&2;   exit 1 ;;
        *[[:space:]]*) echo "Bad input: whitespace" >&2; exit 1 ;;
    esac
    echo "$host"
}

host=$(get_virtual_host)


Codewars kata : reverse and rotate I just started to learn Elixir and to train on Codewars.
I solved this kata but I'm not very satisfied of my code and I have no clue (literally) how to improve it.
Recursion seems the way to go here, but I'm convinced the code can be optimized a little more.

Here is the full problem :

  The input is a string str of digits. Cut the string into chunks of
  size sz (ignore the last chunk if its size is less than sz).

  If a chunk represents an integer such as the sum of the cubes of its
  digits is divisible by 2, reverse it; otherwise rotate it to the left
  by one position. Put together these modified chunks and return the
  result as a string.

Could you help me please ?

defmodule Revrot do
def revrot(str, sz) do
    if str == "" or sz <= 0 or String.length(str) < sz do
      ""
    else
      chunk = str |> String.slice(0,sz)
      t = chunk |> String.to_integer |> Integer.digits |> Enum.reduce(0, fn(x, acc) -> (:math.pow(x,3) |> round) + acc end)
      temp = case (rem t,2) do
        0 -> chunk |> String.reverse
        _ -> String.slice(chunk, 1, sz-1) <> String.first(chunk)
      end
      temp <> revrot(String.slice(str,sz..-1), sz)
    end
  end
end

So, the only major comment I'd have on your code is that it's a bit hard to understand what's going on.  It becomes a lot easier when you have the Kata text to help, but ideally the code itself should be quite easy to understand just by looking at it.

I would split your code up into the seperate stages, rather than having a single function that does everything.  It should make it much easier to see what the code is actually doing at a glance.  There's also a few built in elixir  functions that you can use to simplify things.

So, essentially you have three stages:

Split the string into chunks.
Rotate or reverse each chunk.
Put the string back together.

I would write a function for each of these stages, then you can compose those functions to get the full process.

First, chunking.  You could do this manually, but there's actually already a function that does this in Elixir called Enum.chunk.  So you can convert the string into a list of it's unicode codepoints, chunk it, then join it back into a string, like so:

def chunks(str, sz) do
  str |> String.to_charlist |> Enum.chunk(sz) |> List.to_string
end

Next, we'll need a function to check if we should reverse or not.  You're implementation doesn't seem bad, but:

Elixir provides Enum.sum, which is easier than implementing the sum manually with Enum.reduce.
I would split the sum and the pow operations into two stages.
I wouldn't be afraid to use newlines and whitespace: it can be easier to read than a single line with a lot of operations going on.

So we could write this like:

def cube_digits(chunk) do
  chunk
  |> String.to_integer
  |> Integer.digits
  |> Enum.map(fn (x) -> x |> :math.pow(3) |> round end)
end

def should_reverse?(chunk) do
  (chunk |> cube_digits |> Enum.sum |> rem(2)) == 0
end

Your rotate code seemed fine, but lets pull it into a function:

def rot_left(chunk) do
  String.slice(chunk, 1, -1) <> String.first(chunk)
end

Then you can put this all together into a single function, making use of the into argument to for to join everything back together again:

def revrot(str, sz) do
  for chunk <- chunks(str, sz), into: "" do
    if should_reverse?(chunk) do
      String.reverse(chunk)
    else
      rot_left(chunk)
    end
  end
end

If you were performing some more complicated operations inside the for it might have been worth using pipes along with  Enum.map & Enum.join instead of for, but for this operation I think for ends up nice and concise.


PHP form-to-email script Someone says that my PHP code is vulnerable to XSS. I asked them what I should do to fix it, but now they don't want to help.

$action=$_REQUEST['action'];
                /* send the submitted data */
    {
    $name=$_REQUEST['name'];
    $email=$_REQUEST['email'];
    $message=$_REQUEST['message'];
    if (($name=="")||($email=="")||($message==""))
        {
        echo "All fields are required, please fill the form again.";
        }
    else{
        $from="From: $name<$email>\r\nReturn-path: $email";
        $subject="Message sent using your contact form";
        mail("email@website.com", $subject, $message, $from);
        echo "Email sent!";
        }
    }
?>

Sanitize the user input, but first and foremost: make sure there actually is any user input to sanitize:

$foo = $_POST['bar'];

might issue a notice, if that post variable doesn't exist, that's why it's considered good practice to use isset:

if (!isset($_POST['email']))
    exit('No email address provided');//don't use exit, redirect or something
$email = $_POST['email'];

Next, you sanitize and validate the input. For email addresses, that's easily done:

if (!filter_var($email, FILTER_VALIDATE_EMAIL))
    exit('Invalid email address given');//again, use redirect here

But all the other stuff has to be processed to avoid something you are wide open to:

Mail injection attacks

I've reviewed a couple of code snippets, in detail. Instead of copy-pasting my existing answers here, or typing them a second, third or fourth time, I'll just list a couple of links to some of these answers:

Critique sanitized user email PHP Script (Has most on mail injection, check the links in that answer, too)
Security of a "contact us" form Focusses more on email address validation
How can I make this code safer against XSS attacks? more general on XSS attacks, but make sure to check the links at the bottom.

Side-notes:
If a script contains nothing but PHP code, then the closing ?> tag is best omitted. This advice is given everywhere, including the official php.net site. Google as to why (whitespacing, parser tokens, ...)

I have the feeling you're not showing us the full code, because you have an opening and closing {} around everything, except the first statement.
If this code is the actual code, then please remove those redundant brackets: PHP isn't block-scoped, so they serve no purpouse, other than to confuse anyone else who might ever gaze at your code.


Basic Javascript algorithm for time implementation (morning, afternoon, etc.) I have a small JavaScript time detection module that detects what time it is now, like morning, afternoon, evening, or night.

How can I make the following code less redundant? Is my code already clean? It would also be great if I could get some comments on the style of the code. I really want to know what the best coding practices are.

var now = new Date(),
    hour = now.getHours();

var morning   = (hour >= 4  && hour <= 11),
    afternoon = (hour >= 12 && hour <= 16),
    evening   = (hour >= 17 && hour <= 20),
    night     = (hour >= 21 || hour <= 3);

if(morning) {

  console.log("It's morning");

} else if(afternoon){

  console.log("It's afternoon");

} else if(evening) {

  console.log("It's evening");

} else if(night) {

  console.log("It's night");

}

The first question has to be: What is the point of this? Do you really want to (only) write the results into the console in a real application?

Other than that, it is a bit pointless to pre-calculate the conditions in variables in this case, because they are only used once and they are exclusive to each other.


Summing the Product of Array-member combinations Find the sum of product of all numbers in first array with all the numbers in second array.

Input Format

1st line contains 2 space separated integers N and M.
2nd line contains N space separated integers Ai.
3rd line contains M space separated integers Bi.

Output Format

1 single line containing only the required answer.

#include<iostream>
using namespace std;

int sum(int product)
{
    static int temp = 0;
    temp = temp + product;
    return temp;
}

int main()
{
    int N, M, result, i, j;
    cin>>N>>M;
    int *a1, *a2;
    a1 = new int[N];
    a2 = new int[M];
    for(i = 0; i < N; i++)
        cin>>a1[i];
    for(i = 0; i < M; i++)
        cin>>a2[i];

    for(i = 0; i < N; i++)
    {
        for(j = 0; j < M; j++)
            result = sum(a1[i]*a2[j]);
    }
    cout<<result;
    return 0;
}

Your performance problem here is related to a number of things, but the most significant is the algorithm you use.

Consider the following math:

ax + bx + cx + dx ... == x(a + b + c + d + ...)

Using that logic, the basic math for one array can be simplified.... for example, you can take the second array, sum up the values, and call that sum x.

Now you just need to take all the values in the first array, and multiply them by the sum of the second array, instead of summing the individual products.

As it happens, you can take the same logic, and apply it the other way as well. Bottom line, is that the solution is simply the product of the sums of the arrays:

#include <iostream>

int sum(int count) {
    int tmp, sum = 0;
    for (int i = 0; i < count; i++) {
        std::cin >> tmp;
        sum += tmp;
    }
    return sum;
}

int main() {
    int N, M;
    std::cin >> N >> M;
    int suma = sum(N);
    int sumb = sum(M);
    int result = suma * sumb;
    std::cout << result << "\n";
    return 0;
}

Note, that:

the above code no longer uses "using namespace std;"
The sum method no loger uses static variables. Those are a bad idea especially when it comes to re-entrant code.
Output a newline after the last line.


Switch vs Dictionary logic I'm arguing here with a colleague over two implementations of how to handle a processing logic:

VERSION 1:

public class SomeManager {
    private readonly Dictionary<Type, Action<IData>> dataHandlers;

    public SomeManager() {
        dataHandlers = new Dictionary<Type, Action<IData>> {
            {typeof(DataTypeOne), HandleDataTypeOne},
            {typeof(DataTypeTwo), HandleDataTypeTwo},
            {typeof(DataTypeThree), HandleDataTypeThird}
            //... and so on
        };
    }

    //... could be other code here

    public void Process(IData data) {
        var dataType = data.GetType();

        if (!dataHandlers.ContainsKey(dataType )) {
            Log.Error(string.Format("Unrecognized data type: {0}", dataType ));
            return;
        }

        dataHandlers[dataType ](data);
    }

    private void HandleDataTypeOne(IDispatchData data) {
        //...specific handling here
    }

    private void HandleDataTypeTwo(IData data) {
        //...specific handling here
    }

    private void HandleDataTypeThree(IData data) {
        //...specific handling here
    }
}

VERSION 2:

public class SomeManager {
    public SomeManager() {
    }

    //... could be other code here

    public void Process(IData data) {
        if (data is DataTypeOne) {
            HandleDataTypeOne(data);
        } else if (data is DataTypeTwo) {
            HandleDataTypeTwo(data);
        } else if (data is DataTypeThree) {
            HandleDataTypeThree(data);
        //other cases here
        } else {
           Log.Error(string.Format("Unrecognized data type: {0}", data.GetType()));
        }
    }

    private void HandleDataTypeOne(IDispatchData data) {
        //...specific handling here
    }

    private void HandleDataTypeTwo(IData data) {
        //...specific handling here
    }

    private void HandleDataTypeThree(IData data) {
        //...specific handling here
    }
}

In our case, performance is not an issue.

We are talking here about a specific scenario of 5 data types.

Both approaches are acceptable, but the one using a Dictionary feels cleaner to me, and brings some advantages at no significant cost.

A Dictionary will detect a duplicate key, whereas it could be easily overlooked in an if-else construct, leading to a possibly baffling bug
It could be modified in run-time, eg. for debugging purposes.
It's more flexible: it could be injected from outside, easily refactored towards a DI-based approach etc.

This being said, it could be overengineering for simple cases. It's hard to say without knowing broader context. All info you've provided is that there's 5 cases, which is quite a few already, and it makes me guess there's a chance for more to come; if all I knew was that there's 2 or 3, I would say keep it simple.