Convert Integer to String without using Java API library I have written a Java program that converts an integer to String digit by digit and concatenate them together using the + operator without touching the Stock Java API library.

I like to have feedback on my code. Where do I need to improve. If I need to deduct something. So please criticize me. Thank you.

import java.util.Scanner;

public class StrFromInteger {

    /*
     * a single digit is passed as an argument
     * And a matching digit of String type
     * is returned.
     */
    public static String returnDigitString(int digit) {
        String res = "";

        switch(digit) {
            case 0:
                res = "0"; break;

            case 1:
                res = "1"; break;

            case 2:
                res = "2"; break;

            case 3:
                res = "3"; break;

            case 4:
                res = "4"; break;

            case 5:
                res = "5"; break;

            case 6:
                res = "6"; break;

            case 7:
                res = "7"; break;

            case 8:
                res = "8"; break;

            case 9:
                res = "9"; break;
        }
        return res;
    }

    public static void main(String[] args) {
        // TODO Auto-generated method stub
        //Scan the integer as int
        Scanner scn = new Scanner(System.in);
        int number = scn.nextInt();

        //find the number of digits using logarithm
        //if input number is not equal to zero because
        //log of zero is undefined otherwise if input
        // number zero length is equal to 1
        int input = number;
        int length = 0;
        if(number != 0) {
        length = ( int ) (Math.log10(number) + 1 );}
        else if(number ==0) {
            length = 1;
        }

        //Save each digit in String format by passing
        // the integer digit to the returnDigitString()
        //method one by one
        String[] reverseStr = new String[length];

        String digits = "0123456789";

        int remainder =0;
        int result = number ;
        --length;
        number = length;
        String strSeq = "";
        String valStr = "";

        // loop through the whole integer digit by digit
        //use modulo operator get the remainder
        //save it in remainder. then concatenate valStr
        //returned from returnDigitString()
        //method with previous String of Digits. Divide the result by 10. Again
        //repeat the same process. this time the modulo and the
        //number to be divided will be one digit less at each decremental
        //iteration of the loop.
        for(int i = number; i >= 0; --i) {

            remainder = result % 10;

            valStr = returnDigitString(remainder);
            strSeq = valStr + strSeq;

            result = result / 10;
        }

        //Print the string version of the integer
        System.out.println("The String conversion of " + input + " is: " + strSeq);
    }

}

Personally I think your algorithm has been made a lot more complex than needed.

Is the concatenation a requirement?  If not, you can simplify by directly converting each digit into a char and storing it in a char[].  This way instead of inefficiently concatenating each digit onto the string, you can use the string constructor overload that takes a char[].

With this simplification, the method can be reduced to just a few lines:

  public static String intToString(int num) {
    if(num == 0){
      return "0";
    }
    int count = 0;
    boolean isNeg = false;
    if (num < 0) {
      num *= -1;
      count = 1;
      isNeg = true;
    }
    count += (int) Math.log10(num) + 1;
    char[] digits = new char[count];
    if (isNeg) {
      digits[0] = '-';
    }
    --count;
    while(num > 0) {
      digits[count--] = (char) ((num % 10) + '0');
      num /= 10;
    }
    return new String(digits);
  }


Plain CSS buttons in different sizes Just a plain button, in different sizes. I wanted to achieve exactly the same styling cross browser for both anchors, inputs and buttons.

I'm just wondering if this could be improved.

Markup

<!DOCTYPE html>

        <title>CSS button</title>
        <link rel="stylesheet" href="css/main.css">

        Sign up
        Sign up
        Sign up
        Sign up

        <input type="submit" class="button button-tiny button-primary" value="Sign up">
        <input type="submit" class="button button-primary" value="Sign up">
        <input type="submit" class="button button-medium button-primary" value="Sign up">
        <input type="submit" class="button button-large button-primary" value="Sign up">

        <button class="button button-tiny button-primary">Sign up</button>
        <button class="button button-primary">Sign up</button>
        <button class="button button-medium button-primary">Sign up</button>
        <button class="button button-large button-primary">Sign up</button>

CSS

body,
input,
button {
    font: 14px "Trebuchet MS", sans-serif;
}

input::-moz-focus-inner,
button::-moz-focus-inner {
    border: 0;
    padding: 0;
}

.button {
    padding:8px 16px;
    display:inline-block;
    text-decoration:none;
    border-radius:3px;
    border:0;
    margin:0;
}

input[type="submit"].button,
button.button {
    cursor:pointer;
    outline:none;
}

.button-tiny {
    font-size:12px;
    padding:6px 14px;
}

.button-medium {
    font-size:16px;
    padding:10px 18px;
}

.button-large {
    font-size:18px;
    padding:12px 20px;
}

.button-primary {
    background:slategrey;
    color:#fff;
}

.button-primary:hover {
    background:lightslategrey;
};

I took the liberty of adding your code to a Fiddle Here so that we could see what it being displayed and test it in different browsers.

I tried to set my Browser to IE 8 and less and apparently it doesn't play nice with JSFiddle.  but in IE 9 mode it showed up very nicely, exactly the same as in Chrome.

I like that you created a CSS class button-primary so that you didn't repeat the code you were going to use in all the buttons. and so you didn't have to write so many :hover statements.

Overall I would say that this is some pretty clean code.

But, there is one thing that I would suggest that you do.

Terminate every tag

This

<link rel="stylesheet" href="css/main.css">

Becomes

<link rel="stylesheet" href="css/main.css" />

Notice the /> at the end.  if there is no closing tag you should still "close the tag"

these are called Self closing Tags.

some Tags are self closing tags and some are not. HTML5 is a little blurry on this, or I am a little blurry on HTML5

either way you should make sure that these tags

<input />
<area />
<link />
<meta />

and some others.   This Answers Lists some more

XHTML allows you to Self Close any tag, this isn't really good practice, mostly because most tags are meant to be containers and leaving them empty would be really silly.

This more recent answer from BoltClock explains a little bit better about closing tags.

I recommend always closing your tags, even if the Doctype tells you that you don't have to. it is better practice.


Removing getters for a custom enum Recently, I perform re-factor on one of my frequent used custom enum, by removing its getter.

My rational for doing so it.

The enum's custom fields are immutable
Only use getters when it is necessary.
Less typing. Reduce verbosity.
This is a very stable class, with only 17 commits, since 2008. Its getters behavior had never been changed. It merely perform dummy return on its internal data, without any additional logic. (History of Country.java)

Before Refactor

public enum Country {
    Australia("/images/16x16/au.png", "Australia", Currency.getInstance("AUD")),
    Austria("/images/16x16/at.png", "Austria", Currency.getInstance("EUR")),
    ...

    Country(String fileName, String humanReadableString, Currency currency) {
        this.icon = new javax.swing.ImageIcon(this.getClass().getResource(fileName));
        this.humanReadableString = humanReadableString;
        this.currency = currency;
    }

    public ImageIcon getIcon() {
        return icon;
    }

    public String toHumanReadableString() {
        return humanReadableString;
    }

    public Currency getCurrency() {
        return currency;
    }

    private final ImageIcon icon;
    private final String humanReadableString;
    private final Currency currency;
}

After Refactor

public enum Country {
    Australia("/images/16x16/au.png", "Australia", Currency.getInstance("AUD")),
    Austria("/images/16x16/at.png", "Austria", Currency.getInstance("EUR")),
    ...

    Country(String fileName, String humanString, Currency currency) {
        this.icon = new javax.swing.ImageIcon(this.getClass().getResource(fileName));
        this.humanString = humanString;
        this.currency = currency;
    }

    public final ImageIcon icon;
    public final String humanString;
    public final Currency currency;
}

I was wondering, do you think this is a good re-factoring approach? Would you accept such re-factoring?

I guess your question is essentially:
why do you need getters for final fields,
rather than accessing them directly by name.

One reason is that you won't be able to create custom subclasses that extend the functionality of those fields,
because variables cannot be overridden,
only methods.

This may seem a moot point with an enum as in your question,
because enums cannot be extended anyway.
The most extensible setup is when you work with interfaces.
You could have a Country interface that includes those getters,
and an enum that implements that interface and those getters.
Then you could have a CustomCountry that implements the same interface.
Of course this extensible setup requires methods instead of variables.

Another reason to use getters instead of variables is that the member variables are too intimate implementation details.
In the future you might want to derive these values from multiple other variables,
but you won't be able to without disrupting your users,
since the original variable names are now part of the public API of your class/enum.
By exposing getters instead of the variables directly,
you are free to refactor your internal implementation without disrupting your users.


Lispy code? Prime factors of a number My answer to Project Euler problem 3 (largest prime factor of 600851475143) is below.
Please suggest improvements, style changes, indentation, commenting, naming of "objects", ...
(defun factors (n)
    "return a list with prime factors of n"
    (let ((result))
        (loop while (zerop (rem n 2))           ; do 2
              do (loop while (zerop (rem n 2))
                       do (setf n (/ n 2))
                       do (setf result (cons 2 result))))
        (loop for i from 3 by 2                 ; do 3, 5, 7, 9, ...
              while (>= n (* i i))              ; until square root of what's left of n
              do (loop while (zerop (rem n i))
                       do (setf n (/ n i))
                       do (setf result (cons i result))))
        (if (= n 1) result (cons n result))))   ; if n is not 1 it is also a factor

Questions:

Is the (let ((result)) #| ... (setf result ...) ... |# result) construction acceptable?
I like the result as it is, in reverse; but, other than (reverse result) is there a way to collect factors into result? My attempts with collect i (or collect i into result) would not compile.

Your code is perfectly fine, except for a few nitpicks.

Use push instead of (setf x (conx y x)) for readability.

Use floor instead of rem + / for efficiency.

1st and 3rd loops are identical, so they should be abstracted into a local flet function.

Use nreverse instead of reverse for the return value.  Yes, you can avoid that by using collect combined with nconc but it makes little sense in your specific case.


Suggestions for a Dungeons and Dragons-like game This game is similar to a Dungeons and Dragons game. How could I improve the efficiency of the program?  For example, where can I add functions, records etc?  This is for a school Controlled Assessment so any help would really be appreciated.

import random
play1 = True

str1 = int(input("Enter a value for strength"))
print("you have chosen",str1,)

skl1=int(input("Please enter a value of skill, and then press enter"))
print("you have chosen: ",skl1,)

str2 =int(input("Enter a value for strength"))
print("you have chosen: ",str2,)

skl2=int(input("Please enter a value of skill, and then press enter"))
print("you have chosen: ",skl2,)

strdif=str1-str2
    if strdif<0:
    strdif=str2-str1
    strdif=strdif//5

skldif=skl1-skl2
    if skldif<0:
    skldif=skl2-skl1
    skldif=skldif//5

while play1 == True:

   num1=random.randint(1,6)
   num2=random.randint(1,6)

   if num1>num2:
        str1+=strdif
        skl1+=skldif
        str2-=strdif
        skl2-=skldif

    if num2>num1:
        str2+=strdif
        skl2+=skldif
        str1-=strdif
        skl1-=skldif

    if skl1<0:
        skl1=0
    if skl2<0:
        skl2=0

    if str1<=0:
        play1 = False
        print("Character 2 Won the game")
   elif str2<=0:
        play1 = False
        print("Character 1 Won the game")

   if str1 > str2:
       print("Character 1 won the encounter, now has",str1,"strength and",skl1,"skill")
       print("Character 2 lost the encounter, now has",str2,"strength and",skl2,"skill")
   else:
       print("Character 2 won the encounter, now has",str2,"strength and",skl2,"skill")
       print("Character 1 lost the encounter, now has",str1,"strength and",skl1,"skill")

There are two huge improvements you can make to increase readability of your code:

Choose better names for some variables – skill1 is better than skl1, and strength2 is better than str2 (str usually means “string”, which adds even more confusion). play1 should be play or play_on.
Put whitespace around operators rather than trying to write your code as compact as possible. For example, change

strdif=str1-str2
if strdif<0:
    strdif=str2-str1
strdif=strdif//5

to

strength_difference = strength1 - strength2
if strength_difference < 0:
    strength_difference = strength2 - strength1
strength_difference = strength_difference // 5

Next, there are a number of small issues that could be improved:

while play1 == True could be abbreviated to while play1. However, setting a variable to False in order to exit a loop isn't very elegant: You could break out of the loop instead, or put the loop into a function from which you can return.
When calculating the differences of strength and skill, you could use the abs function instead: strength_difference = abs(strength1 - strength2) // 5.
You roll two random numbers from 1–6, then compare whether they are greater or smaller than each other. Instead, we could directly figure out the probabilities:

2/12: draw
5/12: player 1 deals a blow
5/12: player 2 deals a blow

which means we only have to roll one value from 1–12
Instead of naming our variables foo1 or foo2, let's create dictionaries that hold variables for each player:

player1 = dict()
player1["name"] = "Character 1"
player1["strength"] = ...
player1["skill"] = ...

This looks more verbose, but allows us to remove a lot of duplication in a minute.
We can stuff the character initialization into a function that returns a player dictionary:

def generate_character():
    character = dict()
    character["name"] = raw_input("name: ")
    character["strength"] = int(raw_input("strength: "))
    character["skill"] = int(raw_input("skill: "))

    print("Your character is %s (strength: %d, skill: %d)" % (character["name"], character["strength"], character["skill"]))

    return character

Note that I use raw_input instead of input, as this avoids executing the input as if it were code.

We can then initialize the players like this:

player1 = generate_character()
player2 = generate_character()

Because all info about a player is conveniently bundled, we can do neat stuff like this:

(loser, winner) = sorted((player1, player2), key = lambda player: player["strength"])

which sorts the players by their strength. This simplifies printing out the stats at the end of a round:

for player, status in [(winner, "won"), (loser, "lost")]:
    print("%s %s the encounter, now has %d strength and %d skill" %
          (player["name"], status, player["strength"], player["skill"]))

Using dictionaries for this is easy, but awkward. Instead we could write a class that represents characters, and create one instance of this class for each player:

class Character:
    def __init__(self, name, strength, skill):
        self.name = name
        self.strength = strength
        self.skill = skill

Our generate_character changes to this:

def generate_character():
    name = raw_input("name: ")
    strength = int(raw_input("strength: "))
    skill = int(raw_input("skill: "))

    character = Character(name, strength, skill)

    print("Your character is %s (strength: %d, skill: %d)" % (character.name, character.strength, character.skill))

    return character

If we combine all these points (and a few other minor quibbles), we might end up with something like the following code:

import random

class Character:
    def __init__(self, name, strength, skill):
        self.name = name
        self.strength = strength
        self.skill = skill

    def hit(self, other):
        strength_difference = abs(self.strength - other.strength)
        self.strength += strength_difference
        other.strength = max(0, other.strength - strength_difference)

        skill_difference = abs(self.skill - other.skill)
        self.skill += skill_difference
        other.skill = max(0, other.skill - skill_difference)

def generate_character():
    name = raw_input("name: ")
    strength = int(input("strength: "))
    skill = int(input("skill: "))

    character = Character(name, strength, skill)

    print("Your character is %s (strength: %d, skill: %d)" % (character.name, character.strength, character.skill))

    return character

def run(player1, player2):
    while True:
        print("Encounter!")

        roll = random.randint(1, 12)
        if roll <= 5:
            player1.hit(player2)
        elif roll <= 10:
            player2.hit(player1)
        else:
            pass # draw

        if player1.strength <= 0:
            print("%s won the game" % player1.name)
            return
        if player2.strength <= 0:
            print("%s won the game" % player2.name)
            return

        (loser, winner) = sorted((player1, player2), key = lambda player: player.strength)
        for player, status in [(winner, "won"), (loser, "lost")]:
            print("%s %s the encounter, now has %d strength and %d skill" %
                  (player.name, status, player.strength, player.skill))

if __name__ == "__main__":
    print("Initialize the first character:")
    player1 = generate_character()
    print("Initialize the second character:")
    player2 = generate_character()
    run(player1, player2)


Python traverse a directory recursively and print contact numbers I am writing a python code to recursively traverse a directory containing only text files and then print the 10 digit Indian phone number and all its variants. I am very new to python and I have written the following code to print the numbers and this code is working fine. I want to know if there is any other better way to do this program so that the code looks better. Right now, I have used 3 nested for loops, I was wondering if I could reduce time complexity a bit. I am a beginner in python.

Contact_finder.py

import os, re

class ContactFinder:

    pattern = re.compile('^(?:(?:\+|0{0,2})91(\s*[\ -]\s*)?|[0]?)?[789]\d{9}|(\d[ -]?){10}\d$')

    def traverse(self,dir_path):
        for root, dirs, files in os.walk(dir_path):
            for file in files:
                filename = os.path.join(root, file)
                with open(filename) as fn:
                    for contact in fn:
                        if self.pattern.match(contact):
                            print contact

contact = ContactFinder()
contact.traverse('/home/sinscary/Documents/niki')

Please suggest me a better solution than this.

You don't need a class here at all. You only seem to use it as a namespace and for that just importing it from another file would be sufficient.

I would make pattern a global constant and give it a better name.

I would put the extracting of the phone numbers into a separate function for readability.

I would make both functions generators and make it the responsibility of the caller to actually print the contact (or do something else with it).

Lastly, I would use a if __name__ == "__main__": guard to allow importing these functions without executing the traversal.

import os, re class

PHONE_NUNBER = re.compile('^(?:(?:\+|0{0,2})91(\s*[\ -]\s*)?|[0]?)?[789]\d{9}|(\d[ -]?){10}\d$')

def get_contacts(filename, pattern):
    with open(filename) as fn:
        for contact in fn:
            if pattern.match(contact):
               yield contact

def get_contacts_recursive(dir_path, pattern):
    for root, dirs, files in os.walk(dir_path):
        for file in files:
            filename = os.path.join(root, file)
            for contact in get_contacts(filename, pattern):
                yield contact

def main():
    for contact in get_contacts_recursive('/home/sinscary/Documents/niki', PHONE_NUMBER):
        print contact

if __name__ == "__main__":
    main()


Triangle-numbers with lots of divisors (Project-Euler Problem 12) The following is my python solution of project euler 12.

  The sequence of triangle numbers is generated by adding the natural
  numbers. So the 7th triangle number would be 1 + 2 + 3 + 4 + 5 + 6 + 7
  = 28.
  The first ten terms would be:

  1, 3, 6, 10, 15, 21, 28, 36, 45, 55, ...

  Let us list the factors of the first seven triangle numbers:

 1: 1
 3: 1,3
 6: 1,2,3,6
10: 1,2,5,10
15: 1,3,5,15
21: 1,3,7,21
28: 1,2,4,7,14,28

  We can see that 28 is the first triangle number to have
  over five divisors.

  What is the value of the first triangle number to have over five
  hundred divisors?

It works fine and runs in half second. I fixed a random upper limit for the search of primes (using the sieve of eratosthenes).

def SE(n):                  #Sieve of Eratosthenes
    sieve = range(3, n, 2)
    top = len(sieve)
    for si in sieve:
        if si:
                bottom = (si*si - 3)/2
                if bottom >= top:
                    break
                sieve[bottom::si] = [0] * -((bottom-top)//si)
    return [2] + filter(None, sieve)

primes = SE(13000)        #Here's the problem

def nod(n):                 #function that returns the number of divisors
    nd = 1
    for i in primes:
        if i <= n:
            t = 1
            while n % i == 0:
                n /= i
                t += 1
            nd *= t
        else:
            return nd

c = 0
i = 1
d = 1
d1 = 0
while c < 500:                 #loop for checking each triangle number
    if i % 2 == 0:
        d = nod(i+1)
    else:
        d1 = nod((i+1)/2)
    c = d * d1
    i += 1
print i*(i-1)/2

I'm going to focus on the coding style of your question, as there something to be said on that regard. On the subject of improving or alternate solution on how to do the Project Euler #12, see result of search here on Code Review for euler 12 [python].

Use better names for functions! – If you change your function names to sieve_of_eratoshenes and number_of_divisors you can remove the comments , and instead add proper docstrings explaining more on what the functions does or returns.
Use better names for variables – What are the c, d, d1´, andi` variables? They don't convey their meaning, which they really should.
Avoid top level code inbetween functions – Between your functions you suddenly do primes = SE(13000), and then proceed to use this a global within nod(). Both are code smells. You should gather all of your top level code into a main(), pass the primes as a parameter and call main() like in the following:

import itertools

MAX_FACTOR = 13000
MAX_NUMBER = MAX_FACTOR ** 2

def main():
    primes = sieve_of_erasthones(MAX_FACTOR)

    divisor_count = 0

    for i in itertools.count(1):
        if i > MAX_NUMBER:
            raise ValueError("Will fail as we don't have enough primes!")

        if i % 2 == 0:
           even_divisor_count = number_of_divisors(i+1)
        else:
             odd_divisor_count = number_of_divisors((i+1)//2)

        divisor_count = even_divisor_count * odd_divisor_count

        if divisor_count >= 500:
            break

    print(i*(i-1)/2)

if __name__ == '__main__':
    main()

Do note that this code is untested, and I'm mostly unsure on what the i is and whether i > MAX_NUMBER is the correct test
Possibly add an extension of primes table – Like suggested with i > MAX_NUMBER you should add some sort of test to handle the situation when you run out of primes, and possibly build a method to extend the primes further. This could be done tracking current high, and reiterate over primes already found extending beyond the current high to the double of previous high or similar
Loop on the loop counter – As seen in the code above, I've changed your loop into looping on the i instead of resorting to manually updating the i within the while loop. To still maintain the same functionality, I've added the break statemen as a termination clause on the loop instead.
BUG: Your nod() function will return None if n > max(primes) – Given that the highest number of primes is less than n your code will terminate the loop and return None.
Is it correct to do nd *= t? – I haven't tested, and neither fully grasped your nod() function, but shouldn't it add t to the total number of divisors, and not multiply it?

So to summarize, I think you need to work a little on naming your variables and functions, and add a check for when you code will fail to prevent it producing false and erroneous results. And possibly add a method to extend the primes list if going beyond already calculated primes.


Sudoku type grid generator I'm trying to make a program which prints different sized grids (n by n grids).
These grids cannot have the same number in any column or row (like a Sudoku).
My bruteSolve() method runs through every combination and then isValid() checks if the rows and columns add up & multiply up to a certain number too - if they do, then the program prints the grid.

However, this program takes too long and I want one where I could do say, 10 * 10 grids fairly quickly. How would I be able to change the code so it does it much faster?

import java.util.ArrayList;
import java.util.Arrays;

public class MagicSquareSolver2 {

static ArrayList<Integer> numbers;

static int n;

static int [] [] grid;

static int count;

public static void main (String [] args){

    n = 4;

    grid = new int [n] [n] ;

    bruteSolve(0,0,n);
}

public static void bruteSolve(int a, int b, int n){

    for  (int i=1; i<n+1; i++){
        grid [a][b] = i;
        if (b==n-1 && a==n-1){
            if (isValid(grid)){count++;
            System.out.println(Arrays.deepToString(grid));System.out.println(count);
        }
        }
        else if (b==n-1){
            bruteSolve(a+1,0,n);
        }
        else{
            bruteSolve(a, b+1,n);
        }
    }

}

public static boolean isValid (int [] [] grid) {

    int factorial = 1;
    int fibonacci = 0;
    int totalX1 = 1;
    int totalY1 = 1;
    int totalX2 = 0;
    int totalY2 = 0;

    for (int i=n; i>0; i--){
        factorial = factorial * i;
    }

    for (int i=n; i>0; i--){
        fibonacci = fibonacci +i;
    }

    for (int i=0; i<n; i++){
        for (int j=0; j<n; j++){
            totalY1= totalY1 * grid[i][j]; //checks all columns
            totalX1= totalX1 * grid[j][i]; // checks all rows
            totalX2 = totalX2 + grid[j][i];
            totalY2 = totalY2 + grid[i][j];
        }
        if (totalX1 != factorial || totalY1 != factorial || totalX2 != fibonacci || totalY2 != fibonacci) { return false; }
        totalX1 = 1;
        totalY1 = 1;
        totalX2 = 0;
        totalY2 = 0;
    }

    return true;

}

}

A couple of things:

Indentation

Indentation seems a little off in your question. Is this because of formatting issues when copy and pasting from your IDE, or is it your code? Most IDEs have a format function that formats the code for you. In eclipse, that is found in Source -> Format or the keyboard shortcut Ctrl+Shift+F.

Spacing

You have what I call overspacing:

static int [] [] grid;

and underspacing:

for  (int i=1; i<n+1; i++){

Again, formatting in an IDE will usually fix that.

After formatting, your code will look like:

import java.util.ArrayList;
import java.util.Arrays;

public class MagicSquareSolver2 {

    static ArrayList<Integer> numbers;

    static int n;

    static int[][] grid;

    static int count;

    public static void main(String[] args) {

        n = 4;

        grid = new int[n][n];

        bruteSolve(0, 0, n);
    }

    public static void bruteSolve(int a, int b, int n) {

        for (int i = 1; i < n + 1; i++) {
            grid[a][b] = i;
            if (b == n - 1 && a == n - 1) {
                if (isValid(grid)) {
                    count++;
                    System.out.println(Arrays.deepToString(grid));
                    System.out.println(count);
                }
            } else if (b == n - 1) {
                bruteSolve(a + 1, 0, n);
            } else {
                bruteSolve(a, b + 1, n);
            }
        }

    }

    public static boolean isValid(int[][] grid) {

        int factorial = 1;
        int fibonacci = 0;
        int totalX1 = 1;
        int totalY1 = 1;
        int totalX2 = 0;
        int totalY2 = 0;

        for (int i = n; i > 0; i--) {
            factorial = factorial * i;
        }

        for (int i = n; i > 0; i--) {
            fibonacci = fibonacci + i;
        }

        for (int i = 0; i < n; i++) {
            for (int j = 0; j < n; j++) {
                totalY1 = totalY1 * grid[i][j]; // checks all columns
                totalX1 = totalX1 * grid[j][i]; // checks all rows
                totalX2 = totalX2 + grid[j][i];
                totalY2 = totalY2 + grid[i][j];
            }
            if (totalX1 != factorial || totalY1 != factorial
                    || totalX2 != fibonacci || totalY2 != fibonacci) {
                return false;
            }
            totalX1 = 1;
            totalY1 = 1;
            totalX2 = 0;
            totalY2 = 0;
        }

        return true;

    }

}

After some edits in spacing that are not fixed by the IDE:

import java.util.ArrayList;
import java.util.Arrays;

public class MagicSquareSolver2 {

    static ArrayList<Integer> numbers;
    static int n;
    static int[][] grid;
    static int count;

    public static void main(String[] args) {
        n = 4;
        grid = new int[n][n];
        bruteSolve(0, 0, n);
    }

    public static void bruteSolve(int a, int b, int n) {
        for (int i = 1; i < n + 1; i++) {
            grid[a][b] = i;
            if (b == n - 1 && a == n - 1) {
                if (isValid(grid)) {
                    count++;
                    System.out.println(Arrays.deepToString(grid));
                    System.out.println(count);
                }
            } else if (b == n - 1) {
                bruteSolve(a + 1, 0, n);
            } else {
                bruteSolve(a, b + 1, n);
            }
        }
    }

    public static boolean isValid(int[][] grid) {
        int factorial = 1;
        int fibonacci = 0;
        int totalX1 = 1;
        int totalY1 = 1;
        int totalX2 = 0;
        int totalY2 = 0;

        for (int i = n; i > 0; i--) {
            factorial = factorial * i;
        }
        for (int i = n; i > 0; i--) {
            fibonacci = fibonacci + i;
        }

        for (int i = 0; i < n; i++) {
            for (int j = 0; j < n; j++) {
                totalY1 = totalY1 * grid[i][j]; // checks all columns
                totalX1 = totalX1 * grid[j][i]; // checks all rows
                totalX2 = totalX2 + grid[j][i];
                totalY2 = totalY2 + grid[i][j];
            }
            if (totalX1 != factorial || totalY1 != factorial
                    || totalX2 != fibonacci || totalY2 != fibonacci) {
                return false;
            }
            totalX1 = 1;
            totalY1 = 1;
            totalX2 = 0;
            totalY2 = 0;
        }
        return true;
    }

}

static variables

No, no, no. Rarely should you use static variables, such as a public static final class constant, or something that needs to be static when there is no other option available. You can easily redesign your code to not need static variables:

import java.util.Arrays;

public class MagicSquareSolver2 {

    public static void main(String[] args) {
        int n = 4;
        int[][] grid = new int[n][n];
        bruteSolve(0, 0, n, grid);
    }

    public static void bruteSolve(int a, int b, int n, int[][] grid) {
        for (int i = 1, count = 0; i < n + 1; i++) {
            grid[a][b] = i;
            if (b == n - 1 && a == n - 1) {
                if (isValid(grid, n)) {
                    count++;
                    System.out.println(Arrays.deepToString(grid));
                    System.out.println(count);
                }
            } else if (b == n - 1) {
                bruteSolve(a + 1, 0, n, grid);
            } else {
                bruteSolve(a, b + 1, n, grid);
            }
        }
    }

    public static boolean isValid(int[][] grid, int n) {
        int factorial = 1;
        int fibonacci = 0;
        int totalX1 = 1;
        int totalY1 = 1;
        int totalX2 = 0;
        int totalY2 = 0;

        for (int i = n; i > 0; i--) {
            factorial = factorial * i;
        }
        for (int i = n; i > 0; i--) {
            fibonacci = fibonacci + i;
        }

        for (int i = 0; i < n; i++) {
            for (int j = 0; j < n; j++) {
                totalY1 = totalY1 * grid[i][j]; // checks all columns
                totalX1 = totalX1 * grid[j][i]; // checks all rows
                totalX2 = totalX2 + grid[j][i];
                totalY2 = totalY2 + grid[i][j];
            }
            if (totalX1 != factorial || totalY1 != factorial
                    || totalX2 != fibonacci || totalY2 != fibonacci) {
                return false;
            }
            totalX1 = 1;
            totalY1 = 1;
            totalX2 = 0;
            totalY2 = 0;
        }
        return true;
    }

}

What I did with your variables:

static ArrayList<Integer> numbers;

You didn't even use this...
static int n;

Just simply put this in the main method, and added a int n argument to your isValid() method.
static int[][] grid;

Just simply put this in the main method, and added a int[][] grid argument into your bruteSolve() method.
static int count;

You only really need this in your bruteSolve() method; actually just the for loop, so I just put it there.

Naming

What's a? How about b? What is n? Why are they one-letter names?

One-letter variable names are not good, as they are confusing to understand. The only place you should use them is, for example, a for loop counter.

Change:

a into num1
b into num2
n into size (or, optional: remove the n variable completely, and directly use either grid.size or the number itself, or change it into a constant, as I will do)

Final Code

import java.util.Arrays;

public class MagicSquareSolver2 {

    public static final int SIZE = 4;

    public static void main(String[] args) {
        int[][] grid = new int[SIZE][SIZE];
        bruteSolve(0, 0, grid);
    }

    public static void bruteSolve(int num1, int num2, int[][] grid) {
        for (int i = 1, count = 0; i < SIZE + 1; i++) {
            grid[num1][num2] = i;
            if (num2 == SIZE - 1 && num1 == SIZE - 1) {
                if (isValid(grid)) {
                    count++;
                    System.out.println(Arrays.deepToString(grid));
                    System.out.println(count);
                }
            } else if (num2 == SIZE - 1) {
                bruteSolve(num1 + 1, 0, grid);
            } else {
                bruteSolve(num1, num2 + 1, grid);
            }
        }
    }

    public static boolean isValid(int[][] grid) {
        int factorial = 1;
        int fibonacci = 0;
        int totalX1 = 1;
        int totalY1 = 1;
        int totalX2 = 0;
        int totalY2 = 0;

        for (int i = SIZE; i > 0; i--) {
            factorial = factorial * i;
        }
        for (int i = SIZE; i > 0; i--) {
            fibonacci = fibonacci + i;
        }

        for (int i = 0; i < SIZE; i++) {
            for (int j = 0; j < SIZE; j++) {
                totalY1 = totalY1 * grid[i][j]; // checks all columns
                totalX1 = totalX1 * grid[j][i]; // checks all rows
                totalX2 = totalX2 + grid[j][i];
                totalY2 = totalY2 + grid[i][j];
            }
            if (totalX1 != factorial || totalY1 != factorial
                    || totalX2 != fibonacci || totalY2 != fibonacci) {
                return false;
            }
            totalX1 = 1;
            totalY1 = 1;
            totalX2 = 0;
            totalY2 = 0;
        }
        return true;
    }

}

Looks good now!


Bit shifting in Python I am trying to "decode" a file which has every byte shifted its bits.

I have successfully managed to decode this by reading the file, and then byte by byte doing the following (b_int is the integer of the single byte, it will not be 9 bits or 7 bits, always 8 bit):

right shift all bits 1 place b_int >> 1
left shift all bits 7 places b_int << 7
calculate the lower 8 bits of the left_shifted '{0:08b}'.format(left_shifted)[-8:] (and convert this string back to int with int(binary_8_bits, 2))
do an or on the right_shifted, and lower 8 bits of left_shifted right_shifted | lower_8_left_shifted
from int back to byte bytes([shifted])
write the result to a file

All this is working, but is very slow, as it has to process byte for byte done in the for byte in data loop.

My question: is there a faster way of doing this?

Full program:

def shift_bits(b_int):
    return (b_int >> 1) | (int('{0:08b}'.format(b_int << 7)[-8:], 2))

def decode_ss(filename):
    with open(filename, 'rb') as openfile:
        data = openfile.read()
    outputfilename = ''.join(filename.split('.')[:-2]) + '.txt'
    with open(outputfilename, 'wb') as output:
        for byte in data:
            output.write(bytes([shift_bits(byte)]))

filename = r'C:\inputfile.txt.ss'
decode_ss(filename)
print('done')

Since b_int is limited to bytes we can change your code a little to remove the formatting.

The left_shift can only have two states, if b_int & 1 is 1 or 0. And so is the same as ((b_int & 1) << 7). (b_int >> 1) is as simple as it gets, and so this can stay the same.

And so I changed it to:

def shift_bits(b_int):
    return (b_int >> 1) | (int('{0:08b}'.format(b_int << 7)[-8:], 2))

def p_shift_bits(b_int):
    return ((b_int & 1) << 7) | (b_int >> 1)

Which works as intended:

for i in range(1 << 9):
    if shift_bits(i) != p_shift_bits(i):
        print(i)
        break

However, you can still speed it up if you used functools.lru_cache:

@functools.lru_cache(None)
def pc_shift_bits(b_int):
    return ((b_int & 1) << 7) | (b_int >> 1)

These gave the following timings:

>>> timeit.timeit('shift_bits(i)', 'from __main__ import shift_bits; i=183')
1.9397294419024576
>>> timeit.timeit('p_shift_bits(i)', 'from __main__ import p_shift_bits; i=183')
0.4003988483403518
# Yes heavily biased, but if the length of data is greater than 256, it comes into play.
>>> timeit.timeit('pc_shift_bits(i)', 'from __main__ import pc_shift_bits; i=183')
0.15648568020111497


Program to print a triangle of numbers I want to print the following triangle:

1
1  2
1  2  3
1  2  3  4
1  2  3  4  5
1  2  3  4
1  2  3
1  2
1

I tried to do but I cannot avoid two for loops. Is it possible to achieve the result in a single for loop?

 static void printtriangle()
{
    for (int i = 1; i <= 5; i++)
    {
        for (int j = 1; j <= i; j++)
        {
            Console.Write("" + (j));

        }
        Console.WriteLine("");

    }

        for (int k = 4; k >= 0; --k)
        {

            for (int j = 1; j <= k; j++)
            {
                Console.Write("" + (k));

            }
            Console.WriteLine("");
         }

    }

You can do it with one loop that goes from a negative number to a positive, and create a string from a range calculated from the number:

int max = 5;
for (int i = 1 - max; i < max; i++) {
  Console.WriteLine(String.Join(" ", Enumerable.Range(1, max - Math.Abs(i))));
}

That is of course actually two loops, the Range method will create numbers that the Join method loops through.


merge list of strings into one sorted string So I was tasked the following:

  Given a list of strings, output a sorted string that consists of all
  lowercase characters  (a-z) from each string in the list.

  Example

  [aba, xz] -> aabxz

I tried this:

from collections import Counter
from string import ascii_lowercase
def merge(strings):
    counter = Counter()
    for string in strings:
        for char in string:
            counter[char] +=1

    result = []
    for c in ascii_lowercase:
        if c in counter:
            for _ in range(counter[c]):
                result.append(c)
    return "".join(result)

This works on most test cases, but times out on a couple test cases. I am extremely frustrated. How can I  improve the time complexity of this solution?

counter = Counter()
for string in strings:
    for char in string:
        counter[char] +=1

You are flattening your strings list to count each individual characters. For starter, if you were to extract letters individually, you could feed it to the Counter constructor and avoid the += 1 operation. Second, flattening an iterable is best done using itertools.chain.from_iterable:

counter = Counter(itertools.chain.from_iterable(strings))

result = []
for c in ascii_lowercase:
    if c in counter:
        for _ in range(counter[c]):
            result.append(c)
return "".join(result)

Instead of the inner for loop, you can create repeated sequence of characters using c * counter[c]. This also have the neat advantage to produce the empty string ('') when counter[c] is 0, removing the need for the c in counter test:

return ''.join(c * counter[c] for c in ascii_lowercase)

These changes result in a ×4 speedup on my machine.


Scan files and pick overlapped hex codes Problem

Two folders having files with nameEfile that store lines starting with hex codes.

Print those hex codes that overlap in both folders.

Solution

import os
import sys
import re

def process_cache(cache):
    event_code_set = set()
    for file_name, multi_line_content in cache.items():
        if file_name.endswith('Efile'):
            for line in multi_line_content.splitlines():
                line = line.rstrip('\\') # trailing backslash, if exist
                if bool(re.search(r'^0[xX][0-9a-fA-F]+', line)):
                    # Take the hexcode
                    obj = re.search(r'^0[xX][0-9a-fA-F]+', line)
                    event_code_set.add(obj.string[obj.start():obj.end()])
    return event_code_set

def scan_files(dir):
    cache = {}
    for root, dirs, files in os.walk(dir):
        for name in files:
            if name in ('Efile'):
                path = os.path.join(root, name)
                with open(path,'r') as file:
                    cache[path] = file.read()
    return cache

cache1 = scan_files(sys.argv[1])
cache2 = scan_files(sys.argv[2])

cache1_event_code_set = process_cache(cache1)
cache2_event_code_set = process_cache(cache2)

overlap_event_codes = cache1_event_code_set & cache2_event_code_set
print(overlap_event_codes)

For a typical three entries in a file, with comments(#),

0x00010d35 D 11 G 3,0x10009,N R XY.Condition,   "({a 0x40001} == {H 0x166}) && ({a 0x11ff8} == {I 15})","0x0763ffc2 "
# Below event codes are from vendor xyz
0x84900c5 M 22 Y 1,0x03330469,4,5,6,7,8
0x04b60ff6 L 50 U \
          0x04c60e07,102 && ({a 0x11ff8} == {I 15})","0x0763ffc2 "

Picking 0x00010d35, 0x04b60ff6 & 0x84900c5 is the task. Rest of the line is supposed to be ignored

Some entries are multi-line with trailing backslash.

Each file is in megabytes. Total files in both folders - 80

1) Please suggest optimization in the below code, because pattern check is done twice.

        if bool(re.search(r'^0[xX][0-9a-fA-F]+', line)):
            # Take the hexcode
            obj = re.search(r'^0[xX][0-9a-fA-F]+', line)
            event_code_set.add(obj.string[obj.start():obj.end()])

2) Please suggest coding style optimizations for tree walk, cache and command line arguments.

Repeating the match can be avoided by storing it in a variable first:

match = re.search(r'^0[xX][0-9a-fA-F]+', line)
if match:
    event_codes.add(match.group())

Note also that you don't need to strip a trailing \, because it is ignored afterwards anyways and you are only interested in hex codes at the beginnings of the line and all your multilines seem to have some whitespace at the beginning of the continued line. Note that you could have used re.match(r'0[xX][0-9a-fA-F]+', line), instead of re.search, because it always matches only the beginning of the line.

Python also caches your regex, once you have used it, but you may still squeeze out a bit of performance with re.compile.

I also used the match.group() method, which immediately returns the matching part if the string.

I would also not suggest caching the file content of all the files in the directories. This seems like a huge waste of memory. Instead process them one at a time and add it to your event code sets, or even better, just generate a stream of event codes that you can consume with set:

import os
import sys
import re

def event_codes_file(path):
    with open(path) as file:
        for line in file:
            match = re.match(r'0[xX][0-9a-fA-F]+', line)
            if match:
                yield match.group()

def event_codes_dir(dir):
    for root, dirs, files in os.walk(dir):
        for name in files:
            if name.endswith('Efile'):
                path = os.path.join(root, name)
                yield from event_codes_file(path)

if __name__ == "__main__":
    event_codes1 = set(event_codes_dir(sys.argv[1]))
    event_codes2 = set(event_codes_dir(sys.argv[2]))

    overlap_event_codes = event_codes1 & event_codes2
    print(overlap_event_codes)

yield from x was introduced in Python 3.3 and is mostly equivalent to for i in x: yield i.

Also note that name in ('Efile') is almost the same as name == 'Efile', but matches a bit more (like 'E'). Unless of course you meant what you wrote in process_cache, name.endswith('Efile'). I am assuming the latter.

On a file "test", containing your example content, this produces:

set(event_codes_file("test"))
# {'0x00010d35', '0x04b60ff6', '0x84900c5'}


Authorization using Entity Framework in ASP.NET MVC 1. Backstory

I recently starting programming and I found out that Entity Framework works perfect for my small-sized applications due its simplicity.

I've made my custom authorize attribute for MVC controllers and controller methods to check if the current user has a certain role (which is an enum type in my case).

The following code represents my authorize attribute:

public class HasRoleAttribute : ActionFilterAttribute
{
    private Role _role;

    public HasRoleAttribute(Role role)
    {
        this._role = role;
    }

    public override void OnActionExecuting(ActionExecutingContext filterContext)
    {
        var context = new FactoryManagementContext();

        var userName = filterContext.HttpContext.User.Identity.Name;
        var user = context.Users.FirstOrDefault(item => item.UserName == userName);

        var hasRole = user.Role == _role;

        if (user == null || !hasRole)
        {
            // If this user does not have the
            // required permission then redirect to login page
            var url = new UrlHelper(filterContext.RequestContext);
            var loginUrl = url.Content("/Account/Login");
            filterContext.HttpContext.Response.Redirect(loginUrl, true);
        }
    }
}

public enum Role
{
    Engineer,
    Manager,
    Admin
}

2. Question

It works as a charm, but I have only one question: is it necessary to initialize the database context every single time when authorizing a user?

You should not instanciate a new DbContext each time your code go throw your ActionFilter.  What you should do is to use dependency injection and to define an execution scope.

Because you are using .net Framework and not .net core, I advise you to look into DI providers such as Autofac or Ninject.

I advise you to look into why to use DI and think about what execution scope you need (probably perScope() in your case).

Hope it helps.


Java method refactoring: getting the file name or path I have been learning about refactoring and I have this method that I'm wondering if I should split into two for code cleanness. First, the method as one, next the method split in two:

Method not split:

public static ArrayList<String> getFilePathOrName(String pathOrName) {
    ArrayList<String> files = new ArrayList<>();
    File folder = new File("C:\\Users\\damar\\OneDrive\\Documenten\\FolderTest\\Folderretouren\\Niet verdeeld");
    File[] listOfFiles = folder.listFiles();

    if(pathOrName.equalsIgnoreCase("path")) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(file.toString());
            }
        }
    } else if(pathOrName.equalsIgnoreCase("name")) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(file.getName());
            }
        }
    }
    return files;
}

Method split:

public static ArrayList<String> getAllUndistributedFilesNames() {
    ArrayList<String> files = new ArrayList<>();
    File folder = new File("C:\\Users\\damar\\OneDrive\\Documenten\\FolderTest\\Folderretouren\\Niet verdeeld");
    File[] listOfFiles = folder.listFiles();

    for (File file : listOfFiles) {
        if (file.isFile()) {
            files.add(file.getName());
        }
    }
    return files;
}

public static ArrayList<String> getAllUndistributedFilesPaths() {
    ArrayList<String> files = new ArrayList<>();
    File folder = new File("C:\\Users\\damar\\OneDrive\\Documenten\\FolderTest\\Folderretouren\\Niet verdeeld");
    File[] listOfFiles = folder.listFiles();

    for (File file : listOfFiles) {
        if (file.isFile()) {
            files.add(file.toString());
        }
    }
    return files;
}

I feel like the method names when they are split is more intuitive. However, I'm not sure if that is enough justification to have this much duplicate code.

When we refactor the aim is to reduce code duplication. but you introduced even more duplication.

The way to go is to look out for code that is similar and make it the same.

When looking at your original method the duplication is in the if/else cascade. This means you should deal with that. The obvious differences are:

the string literal used
the else before the second if
the method called at the object file

Lets convert that in thee steps:

the else is not needed for business reasons. It is introduced for performance optimization. You can safely remove this else and verify by measurement it that has really an impact. My guess is it does not. so lets let rid of it:

if(pathOrName.equalsIgnoreCase("path")) {
    for (File file : listOfFiles) {
        if (file.isFile()) {
            files.add(file.toString());
        }
    }
}
if(pathOrName.equalsIgnoreCase("name")) {
    for (File file : listOfFiles) {
        if (file.isFile()) {
            files.add(file.getName());
        }
    }
}

now you can extract the first literal string into a variable:

String requestedPathName = "path";
if(pathOrName.equalsIgnoreCase(requestedPathName)) {
    for (File file : listOfFiles) {
        if (file.isFile()) {
            files.add(file.toString());
        }
    }
}
if(pathOrName.equalsIgnoreCase("name")) {
  // ...

Then, after the first if block we can resuse this variable:

String requestedPathName = "path";
if(pathOrName.equalsIgnoreCase(requestedPathName)) {
    for (File file : listOfFiles) {
        if (file.isFile()) {
            files.add(file.toString());
        }
    }
}
requestedPathName = "name";
if(pathOrName.equalsIgnoreCase(requestedPathName)) {
  // ...

The last change requires to use an interface. This interface should declare a method that takes a file object as a parameter and returns a String object. One option is to define such interface yourself:

@FunctionalInterface
interface FileToStringConverter{
     String convert(File file);
}

Or we use the Function interface provides by the Java standard Lib. I choose the latter.

The principle is the same as with the string literal before:

replace the original code with the interface usage:

if(pathOrName.equalsIgnoreCase(requestedPathName)) {
    for (File file : listOfFiles) {
        if (file.isFile()) {
            files.add(new Function<File,String>(){
      @Override public String apply(File t){
           t.toString();
         }
      }.apply(file));
        }
    }
}

extract the interface implementation to a local variable:

// interface implemented as Lambda and method reference.
Function<File,String> converter = File::toString; // interface implemented as Lambda and method reference.
 if(pathOrName.equalsIgnoreCase(requestedPathName)) {
    for (File file : listOfFiles) {
        if (file.isFile()) {
            files.add(converter.apply(file));
        }
    }
}

repeat this in the other `if block:

String requestedPathName = "path";
Function<File,String> converter = File::toString;
  if(pathOrName.equalsIgnoreCase(requestedPathName)) {
    for (File file : listOfFiles) {
        if (file.isFile()) {
            files.add(converter.apply(file));
        }
    }
}
requestedPathName = "name";
converter = File::getName;
if(pathOrName.equalsIgnoreCase(requestedPathName)) {
    for (File file : listOfFiles) {
        if (file.isFile()) {
            files.add(converter.apply(file));
        }
    }
}

Now both if blocks look exactly the same. You can select one of them and apply your IDEs extract method automated refactoring feature:

public static ArrayList<String> getFilePathOrName(String pathOrName) {
    ArrayList<String> files = new ArrayList<>();
    File folder = new File("C:\\Users\\damar\\OneDrive\\Documenten\\FolderTest\\Folderretouren\\Niet verdeeld");
    File[] listOfFiles = folder.listFiles();

        String requestedPathName = "path";
        Function<File,String> converter = File::getName;
        // of cause you should choose a better name for the method!
        extracted(requestedPathName, pathOrName, converter, listOfFiles, files);
        requestedPathName = "name";
        converter = File::getName;
        extracted(requestedPathName, pathOrName, converter, listOfFiles, files);

    return files;
}

private void extracted(
           String requestedPathName,
           String pathOrName,
           Function<File,String> converter,
           File[] listOfFiles,
           List<File> files) {
    if(pathOrName.equalsIgnoreCase(requestedPathName)) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(converter.apply(file));
            }
        }
    }
}

last step is inlineing the variables:

     extracted("path", pathOrName, File::toString, listOfFiles, files);
     extracted("name", pathOrName, File::getName, listOfFiles, files);

The complete result (with a better name for the extracted method) is:

public static ArrayList<String> getFilePathOrName(String pathOrName) {
    ArrayList<String> files = new ArrayList<>();
    File folder = new File("C:\\Users\\damar\\OneDrive\\Documenten\\FolderTest\\Folderretouren\\Niet verdeeld");
    File[] listOfFiles = folder.listFiles();

    addPathOrNameToList("path", pathOrName, File::toString, listOfFiles, files);
    addPathOrNameToList("name", pathOrName, File::getName, listOfFiles, files);

    return files;
}

private void addPathOrNameToList(
           String requestedPathName,
           String pathOrName,
           Function<File,String> fileToStringConversion,
           File[] listOfFiles,
           List<File> files) {
    if(pathOrName.equalsIgnoreCase(requestedPathName)) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(fileToStringConversion.apply(file));
            }
        }
    }
}

We could turn the repeated method call into a loop if we would introduce another interface or an enum to encapsulate the differing data.


Python - Random Number Guessing Game I'm new to programming in general (Python is the first language I've tried learning) and I'm working my way through a collection of beginner's challenges.  I'm having a little trouble wrapping my head around functions, positional arguments, and the way they interact with user input.  I wrote this program to test my understanding.  Is there any way it could be made more efficient?

This program does the following things:

It generates a random 3 digit number.
It tells the user how to play the game.
It asks the user to guess the number.
If the number is too low, it returns 'Higher' and if the number is too high it returns 'Lower'.  Finally, if the user guesses the number correctly, it congratulates the user.
Once the correct number has been guessed, it asks the user if he/she wants to play again.
If the user says yes, the program generates a new random 3 digit number and the game starts again.  If the user chooses no, it says goodbye and quits.

Here's the code:

import random

def instructions():
    """Tells user how to operate program."""
    i = """
    I have selected a random 3 digit number.  Your goal is to guess my
    number in the smallest amount of of moves.  I will tell you if your
    guess is too high or too low, but that's all the help you'll get
    from me.
    """
    print(i)

def guess():
    g = int(input("Guess the number: "))
    return g

def hint(x):
    if x > ans:
        print("Lower")
    elif x < ans:
        print("Higher")
    else:
        print(victory(x))

def victory(x):
    if x == ans:
        print("Congratulations!  You got it!")

instructions()

ans = random.randint(100,999)

while True:
    x = guess()
    if x != ans:
        hint(x)
    elif x == ans:
        victory(x)
        replay = input("Would you like to play again? y/n: ").strip().lower()
        if replay == "y":
            ans = random.randint(100,999)
            continue
        else:
            print("Okay then.  Bye-bye.")
            break

Again, just to clarify, I've tested the program and it does work, I'm just looking to see if there's a more efficient way of writing it which doesn't sacrifice any of its current functions.  Thanks.

I'd say the code itself is not bad but there are a few things to keep in mind.

i is a bad name for a string. Also, you don't need it in the instructions function, you can just print the string.
The same goes for g. It's not a good name and you can return directly the input.
guess is not a good name because the function is not guessing, it's getting the user input, so you could call it get_user_input. Also, since the number is between 100 and 999, you could add some checks that the user input is in that range, and you could use some constants to define the range.
This works correctly on python 3, but on python 2 you would use raw_input instead of input. Just something to keep in mind.
In the hint function you're printing the output of the victory function, which actually is returning nothing. That's why it works, but the proper way would be to either use the victory function only to return the winning message, or to use the victory function separately to check that the user has won. It looks like this is the main point you want to understand, so have a close look at that.
victory is also not a really good name. If you want to check that the user has won, name it has_won, or maybe is_guess_correct.
If the user enters something invalid (like a letter or an empty line) you're going to get an error. You should use exceptions there, but maybe this is a bit of an advanced topic for now?
You don't need the continue in this case, because nothing is executed after that instruction.

Modified code:

import random

LOWER_LIMIT = 100
UPPER_LIMIT = 999

def instructions():
    """Tells user how to operate program."""
    print("""
    I have selected a random 3 digit number.  Your goal is to guess my
    number in the smallest amount of of moves.  I will tell you if your
    guess is too high or too low, but that's all the help you'll get
    from me.
    """)

def get_user_input():
    current_guess = int(input("Guess the number: "))
    while (current_guess < LOWER_LIMIT) or (current_guess > UPPER_LIMIT):
        current_guess = int(input("Please enter a number between " + str(LOWER_LIMIT) + " and " + str(UPPER_LIMIT) + ": "))

    return current_guess

def hint(x):
    if x > ans:
        print("Lower")
    elif x < ans:
        print("Higher")

def is_guess_correct(x):
    if x == ans:
        print("Congratulations!  You got it!")
        return True
    return False

instructions()

ans = random.randint(LOWER_LIMIT, UPPER_LIMIT)

while True:
    x = get_user_input()
    if is_guess_correct(x):
        replay = input("Would you like to play again? y/n: ").strip().lower()
        if replay == "y":
            ans = random.randint(100,999)
        else:
            print("Okay then.  Bye-bye.")
            break
    else:
        hint(x)


Producer/consumer implementation using Tasks and BlockingCollections I'm implementing a Producer/Consumer but the generic interface doesn't require any knowledge of blocking collections or tasks.

public interface IReader
{
    IEnumerable<Data> Read();
}

Is it okay if my implementation of the IReader launches a task asynchronously and returns the consuming enumerable?

public IEnumerable<Data> IReader.Read()
{
    var t = Task.Factory.StartNew(() =>
    {
        BlockingCollection<Data> data = new BlockingCollection();
        DoRead(data);
        return data.GetConsumingEnumerable();
    });

    return t.Result;
}

My concerns are:

Performance.  Is there any negative effect from having the
BlockingCollection on a different thread than the consumer?
Is it bad practice to not indicate that the method is executing asynchronously?

Did you know that Task.Result blocks? Your code isn't asynchronous at all - and it can't be unless DoRead is.

Effectively your code is starting a new task and then blocking while it completes - there's no benefit to starting the task at all!

As a small aside, Task.Run is easier than using Task.Factory.StartNew.

If I were you, I'd just remove the Task.


HTML Templates with Substitutable Areas - PHP In order to separate my views (markup) and code, I elected to write my views like this:

    <!DOCTYPE HTML>

        <title>%PAGE_TITLE%</title>
        <meta charset="UTF-8">
        <link href='PotatoDocs/views/stylesheets/docs2013.css' type='text/css' rel='stylesheet'>
        <link rel="shortcut icon" href="favicon.ico" />

        <link rel="stylesheet" href="highlight.js/styles/default.css">

%PAGE_TITLE%

                %CONTENT%

                %SIDEBAR%

&nbsp;

            Potato Seed 2013

A method then replaces the substitution area with parts of the website, like so:

    private function generatePageSource($page_title)
    {
        $page_template = file_get_contents(REGISTRY_CODE_PATH . "views/templates/" .
            $this->template_dir . "/main.html");

        $page_content = str_replace("%PAGE_TITLE%", pdisplay($page_title), $page_template);
        $page_content = str_replace("%CONTENT%", $this->pagecontent_content, $page_content);
        $page_content = str_replace("%SIDEBAR%", $this->sidebar_content, $page_content);

        $this->page_content = $page_content;
    }

The $this->page_content is later echoed.

Not too relevant to my question, but for the sake of completion, this is the pdisplay() function.

function pdisplay($pdisplay)
{
    $pdisplay = htmlentities(stripslashes($pdisplay), ENT_QUOTES);
    return $pdisplay;
}

My question is this (though I accept any code criticism at all): is this a sensible way to render pages?

I used to implant php tags with echoes onto a page -- but this meant that code (even a little bit) was on the views, so I thought this would be a better solution.

According your comment you don't want to use a template engine, but are actually creating a new one. At some point you will need loops or ifs and it starts becoming ugly.

PHP is already a template engine. Actually it was designed for exactly this purpose. There is a braceless/echoless syntax for all control structures.

Your template/view:

<!DOCTYPE HTML>

    <title><?=$page_title?></title>
    ...

<?=$page_title?>

            <?=$content?>

     ...

Your controller:

private function generatePageSource($page_title)
{
    $page_title=pdisplay($page_title));
    $content = $this->pagecontent_content;

    ob_start();
    $template=REGISTRY_CODE_PATH . "views/templates/" .
        $this->template_dir . "/main.html";
    include $template;
    $this->page_content=ob_get_clean();
}

Of course you could wrap this in a View class to DRY.


Delete node from singly linked list with tail pointer in C I'm trying to implement a single linked list based on Linus' comments about double pointers.

In my case I'm also using a tail pointer and I'm wondering if there's a better way to update the tail pointer which does not require the conditional below?

I'm still new to C so any other critique is appreciated (I do realise I'm not freeing the memory for the node in the delete function, I'm leaving that to the caller in case they want to do something else with the node)

typedef struct lrsll_node {
    struct lrsll_node *next;
    char *data;
} lrsll_node;

typedef struct lrsll_list {
    lrsll_node *head;
    lrsll_node *tail;
} lrsll_list;

lrsll_node *lrsll_delete(lrsll_list **list, char *data) {
    lrsll_node **node = &(*list)->head;
    while (*node && strcmp((*node)->data, data) != 0) {
        node = &(*node)->next;
    }
    if (*node == NULL)
        return NULL;

    lrsll_node *deleted = *node;
    *node = deleted->next;

    //is there a way to get rid of this conditional check
    //using pointers in a more intelligent manner?
    if (deleted == (*list)->tail) {
        (*list)->tail = NULL;
    }

    return deleted;
};

You have not specified the behavior of your list, which makes it hard for us to reason about it, and makes it hard for you to reason about it. Because this is a module about a data structure, it's a good fit for Design-by-Contract, which I recommend you read about.

Fundamentally, in DbC, each method or function has a precondition which must be met just prior to executing the code, a postcondition which must be met after executing the code, and an invariant which must be true at the start and the end of execution, as well as being true when other public methods or functions are called from within the code.

Consider your lrsll_delete(list, data) operation: the precondition is that a node with key data may or may not be present in the list. The postcondition is that the first such node will be removed from the list. (It's not clear whether you want to handle multiple nodes with the same key: do you?)

You don't specify an invariant, but I'd suggest the invariant might be that list->head points to the first node in the list, or is NULL for an empty list, while list->tail points to the last node in the list, or is NULL for an empty list, and each node's ->next field points to the next node in the list, or is NULL when the node is at the end of the list.

You should consider documenting these three conditions. The invariant is always true - you can document it once at the top of the module. You might even document it in code, in a private helper function you can call for debugging purposes: _check_invariant(list).

The pre- and post-conditions will vary from function to function. I suggest documenting them in a block comment.

With that out of the way, let's consider your code:

lrsll_node *lrsll_delete(lrsll_list **list, char *data) {

Right off the bat, I want to ask why you're passing a lrsll_list** pointer? The "double pointer" in question is the lrsll_node**, not _list. Since the _list structure is encapsulating the head and tail pointers, and is not a node itself, I'll recommend that you just use a single level of pointer for this.

I'd also suggest that you typedef the key type. Make it a lrsll_key_t or something, not a char *.

    lrsll_node **node = &(*list)->head;

This can be just &list->head, if you use only one level of pointer to the list structure.

    while (*node && strcmp((*node)->data, data) != 0) {
        node = &(*node)->next;
    }

This is you searching for a matching key in the list. I'll bet you have this same code in other places, like a "find" and maybe an "insert" (if keys are required to be unique). You should consider either defining a macro or a private (internal) function to do this for you.

    if (*node == NULL)
        return NULL;

Not found so return null. This is legit. Moving on:

    lrsll_node *deleted = *node;

Save a pointer to the deleted node. Okay.

    *node = deleted->next;

Update the pointer to that node. Note: It's important to note that there are two possible structures being pointed to by node. In the first case, node is initialized to point to list->head, so node might be pointing into a struct lrsll_list. After the first node, you update with node = &(*node)->next which will be a pointer to a struct lrsll_node. So the node double-pointer points into two possible kinds of struct. That's awkward.

    //is there a way to get rid of this conditional check
    //using pointers in a more intelligent manner?
    if (deleted == (*list)->tail) {
        (*list)->tail = NULL;
    }

To answer your question, there's no good way that I know of - nor even a bad way - to get an update to the list->tail pointer without either destroying it and rebuilding it (by using the tail as your temporary, and then scanning-to-end once you're done), or comparing it and updating it if needed.

Suppose you use tail as some kind of previous node pointer? Well, that's the situation you are determined to avoid with the whole "Linus' double pointer" scenario. So I'll assume you don't want to do that.

Then how can you safely update the tail pointer? Well, consider this:

head -> node1 -> null
tail ---^

In the case where the list contains only one element, what happens if you delete it?

node -> &head -> node1

First, node points to &head. Then *node->data matches, because I said so. So you delete the node by setting *node = deleted->next (which is NULL in this case). So now you have:

node -> &head -> NULL
         tail -> node1

So in this case, your conditional code is correct:

    if (deleted == list->tail)
        list->tail = NULL;

And produces this result:

node -> &head -> NULL
         tail -> NULL

Deleting the 1 node in a 1-node list produces an empty list, which is stored as shown.

But now consider a two node list where the last node is deleted:

head -> node1 -> node2 -> NULL
tail ------------^

After searching, you have:

node -> &(node1->next) -> node2

You set *node = (*node)->next which is NULL, and get:

head -> node1 -> NULL
node ---^.next // node points to &node1.next
tail -> node2

So you run your test:

    if (deleted == list->tail)
        list->tail = NULL;

Which passes, because you did delete the last node in the chain, but now you get:

head -> node1 -> NULL
tail -> NULL

And this is wrong. Tail should point to node1, not to NULL.

How can you fix this? By adding another condition: when updating the tail pointer, check to see if the node pointer is still pointing to &list->head. If so, then go ahead and null it out because you know you have a 1-element list being shortened to an empty list. But if node points to some other location, and you're deleting the tail element, then you can set tail to point to the address of the struct containing the next pointer that is pointed to by node. (Say that five times fast!)

Fortunately, the next pointer is the first element in the struct, so in C you can assume that the two addresses are the same, and just cast one type to the other. (In C++, with a vtable, the addresses wouldn't be the same. In C with any other field at the top of the struct, the addresses wouldn't be the same. That's what the offsetof macro is for! You could subtract the offset, but ... c'mon! This is C! Just hard-code the 0 and move on...)

Something like this:

    if (deleted == list->tail) {
        list->tail = (node == &list->head) ? NULL : (lrsll_node*)node;
    }

Finally:

    return deleted;
};

You don't need that last semi-colon.


Car Rental Optimization Inspired by this r/dailyprogrammer post, the task is too calculate which records to use in order to serve the most records without any of the records having overlapping time spans.

I accomplish this goal by sorting the list of records by their total days spanned, then by iterating over each element and adding it to a new list as long as that record does not overlap with any records already within the "optimized" list.

The source,

internal class Program
{
    private static void Main(string[] args)
    {
        var rentalRecords = RentalRecordLoader.Load("rentals.txt");
        var result = RentalRecordOptimizer.CalculateMostEfficientRecords(rentalRecords);

        Console.WriteLine($"Total Records: {result.Count}");
        result.ForEach(r => Console.WriteLine($"{r.StartDay} : {r.EndDay}"));
    }
}

public class RentalRecord
{
    public int StartDay { get; set; }
    public int EndDay { get; set; }

    public RentalRecord(int startDay, int endDay)
    {
        StartDay = startDay;
        EndDay = endDay;
    }

    public int TotalDays()
    {
        return EndDay - StartDay;
    }

    public bool Overlapping(RentalRecord rentalRecord)
    {
        return StartDay <= rentalRecord.EndDay &&
            rentalRecord.StartDay <= EndDay;
    }
}

public abstract class RentalRecordLoader
{
    public static List<RentalRecord> Load(string path)
    {
        int recordCount;
        int[] startDays;
        int[] endDays;
        using (var reader = new StreamReader(path))
        {
            recordCount = int.Parse(reader.ReadLine());
            startDays = ConvertStringToIntArray(reader.ReadLine());
            endDays = ConvertStringToIntArray(reader.ReadLine());
        }

        var rentalList = new List<RentalRecord>();
        for (var i = 0; i < recordCount; i++)
        {
            rentalList.Add(new RentalRecord(startDays[i], endDays[i]));
        }

        return rentalList;
    }

    // Load() helper method
    private static int[] ConvertStringToIntArray(
        string value, string delimeter = " ")
    {
        return value.Trim().Split(delimeter)
                .Select(x => int.Parse(x))
                .ToArray();
    }
}

public abstract class RentalRecordOptimizer
{
    public static List<RentalRecord> CalculateMostEfficientRecords(
        List<RentalRecord> rentals)
    {
        var efficientRentals = new List<RentalRecord>();
        foreach (var rental in rentals.OrderBy(x => x.TotalDays()))
        {
            // Cant have any overlapping days.
            // The car cant be in two places at once.
            if (efficientRentals.Any(x => x.Overlapping(rental)))
            {
                continue;
            }
            efficientRentals.Add(rental);
        }
        return efficientRentals;
    }
}

The input used (stored in rentals.txt)

10
1 2 5 12 13 40 30 22 70 19
23 10 10 29 25 66 35 33 100 65

And the output:

Total Records: 5
5 : 10
30 : 35
13 : 25
40 : 66
70 : 100

Optimizations and better code design is obviously welcome, but I also had a specific question as well.

What would be the best way to handle incorrectly formatted data in
rentals.txt? Just catch the exception in the Load function?

Before answering the question I have a few convention suggestions for you:

Use static instead f abstract when creating static classes.
By the look of your code I assume that your classes are not mean to be used for creating new instances but using abstract to achieve this is ill advised since you can walk around the restriction by creating a class which inherits from this class. Instead use the static keyword to create your static class:

public static class MyStaticClass
{
    //static members here
}

The method total days could be refactored as a property just with a getter method like this:

public int TotalDays => EndDay - StartDay;

You could also use the Linq method Zip() to join the input lines into the record object

With Zip() you can turn this

var rentalList = new List<RentalRecord>();

for (var i = 0; i < recordCount; i++)
{
    rentalList.Add(new RentalRecord(startDays[i], endDays[i]));
}

return rentalList;

into this:

return startDays.Zip(endDays, (startDay, endDay)
    => new RentalRecord(startDay, endDay));

Now for your question:
I suggest validating the content of the file instead of letting it fail without having code explaining why it fail and then throw your own custom exception when the validation method fails. I'd refactor your Load method like this:

public static List<RentalRecord> Load(string path)
{
    int recordCount;
    int[] startDays;
    int[] endDays;
    using (var reader = new StreamReader(path))
    {
        if(!int.TryParse(out recordCount))
        {
            throw new MyInvalidCustomException("Record line is not in the
            correct format);
        }
        startDays = ConvertStringToIntArray(reader.ReadLine());
        endDays = ConvertStringToIntArray(reader.ReadLine());
        if(startDays.Length != endDays.Length)
        {
            throw new MyInvalidCustomException("not all records have
            matching start and end days");
        }
    }

    return startDays.Zip(endDays, (startDay, endDay)
    => new RentalRecord(startDay, endDay));
}

This gives you a more specific exception control so you can troubleshoot the errors better.


Multiple string interpolation Mostly robocop and hound give "line is too long" for such code statements. I just wanted to know which one is better and preferable way.

This one is a way to avoid making the message long so just do multiple string interpolation:

timeGet = "after #{getTime2(online, offline)}"

logs[index].extra = {message: "Camera came online #{timeGet}"}

This next one is to not do multiple string interpolation but just expand the JSON object to the new line to avoid "line is too long". What do you think is a good and preferable way of coding?

logs[index].extra = {
  message: "Camera came online after #{getTime2(online, offline)}"
}

For long strings in CoffeeScript consider using the heredocs notation:

logs[index].extra = {
  message: """
           Camera came online

             after #{getTime2(online, offline)}

           """
}

or simply concatenate multiple lines:

logs[index].extra = {
  message: "Camera came online" +
           "" +
           "  after #{getTime2(online, offline)}" +
           ""
}

The extra newlines and multiple spaces shouldn't matter in HTML since they're all trimmed to one space by the renderer (except defined otherwise, e.g. in pre or code).

It wasn't your question, but please also consider this syntax for defining an object and adding a field to it. I think it's better readable due to less indentation, it's more explicit and doesn't make anyone thing you'd add on top of an existing object:

logs[index].extra = {}
logs[index].extra.message = "Camera came online" +
                            "" +
                            "  after #{getTime2(online, offline)}" +
                            ""

The code from this example is equivalent to the code above it.


Reading and processing a big CSV file I am trying to write a script to read a CSV file containing 1 million domain names, look up these domain names and store the results in another CSV file. I am trying the following code, but the number of records I am able to parse with time is quite less. I want to optimize the performance of this to be able to parse more records in less amount of time and also keep the CPU utilization under control. What all should I be focusing on or where can I get help regarding the same:

package main

import (
    "fmt"
    "io"
    "time"
    "net"
    "os"
    "encoding/csv"
    "strings"
)

func main() {

    resc, errc := make(chan string), make(chan error)

    fmt.Println("start time", time.Now())

    concurrency := 1000
    sem := make(chan bool, concurrency)

    csvfile, err := os.Open("1-million-rows.csv")

    if err != nil {
        fmt.Println(err)
        return
    }

    defer csvfile.Close()
    reader := csv.NewReader(csvfile)
    reader.FieldsPerRecord = -1 // see the Reader struct information below
    row_count := 0
    for {
        sem <- true
        record, err := reader.Read()
        if err == io.EOF {
            break
        }
        row_count += 1
        // Stop at EOF.
        domain_name := record[1]
        go func(domain_name string) {
            defer func() { <-sem }()
            body, err := lookup(domain_name)
            if err != nil {
                errc <- err
                return
            }
            resc <- string(body)
        }(domain_name)

    }

    for i := 0; i < cap(sem); i++ {
        sem <- true
    }

    for i := 0; i < row_count; i++ {
        select {
        case res := <-resc:
            fmt.Println(res)
        case err := <-errc:
            fmt.Println(err)
        }
    }
}

func lookup(domain_name string) (string, error) {
  ip, err := net.LookupIP(domain_name)
  if err != nil {
    return "", err
  }
  var ip_addresses []string
  for i := range ip{
    address := ip[i]
    ip_addresses = append(ip_addresses, address.String())
  }
  row := domain_name + ",[" + strings.Join(ip_addresses,":") + "]," + time.Now().String()
  fmt.Println(row)
  return row, nil
}

I am running the script and redirecting the output to another file (which will contain the output records that I need). Another problem that I am facing is that the program almost stops (zero CPU utilization) after a few seconds and after printing nearly 600 lines. Is there anything wrong with the way I am controlling concurrency?

Bugs
Your code does have a problem with the concurrency, and it's a big one.... your buffered channels are not going to be able to complete the system. The issue is the unbuffered resc and errc channels:

resc, errc := make(chan string), make(chan error)

Your code attempts to loop through a million input records, but your concurrency is set relatively low at 1000. After you have looped a thousand times, you have 1000 go-routines all looking to write a result to either the resc or errc channels.... but, because those channels are unbuffered, there needs to be something reading from them before they can be written to...
... but you have to queue up all 1000000 CSV records before you start to read those channels.
Because you never complete the loops (you're blocked writing to the resc channel), you never release a value from the sem either.... thus, you never get to progress in the loop.
Your code is buggy.
A solution would be to put the entire CSV-reading code in to a separate go-routine, so that your can start reading from the resc/errc channels immediately.
struct instead
Instead of having 2 return channels errc and resc, it would be better to return a simple struct:
type dnsLookup struct {
    domain string
    ips    []string
    err    error
}

then you can monitor just a single channel, and can also correlate any results to a name in a better way, and also correlate an error better too.
Concurrency
The use of the sem channel is OK for concurrency, but for a job like this I would instead defer to having a more discrete mechanism. Have a channel that you push domain names on to from the CSV parser, and then have X number of concurrent go-routines reading from that. Closing the channel indicates no-more-data to process. Use wait-groups to monitor completion....
func lookupRoutine(source <-chan string, wg *sync.Waitgroup, results chan dnsLookup) {
    defer wg.Done()
    for name := range source {
        results <- lookup(name)
    }
}

The above loop will process any/all available names from the source (until it is closed), and will then send values to the results channel.
Then, in your main loop, you can have a channel to send CSV parse results to:
names := make(chan string, 1000)
results := make(chan dnsLookup, 1000)

// parse names in a goroutine
go parseCSVData(csvfile, names)

wg := new(sync.WaitGroup)
wg.Add(concurrency)
for I := 0; i < concurrency; i++ {
    // parallel routine for lookups
    go lookupRoutine(names, wg, results)
}

// close the results when all lookup routines complete:
go func() {
    wg.Wait()
    close(results)
}

for r := range results {
    // print the results out here
    ...
}


compute_ranks function I've been working on optimizing such as loop unrolling, is there anything I can do to optimize this code to make it run faster/in less cycles?

void compute_ranks(float *F, int N, int *R, float *avg, float *passing_avg, int *num_passed) {
    int i, j;
    *num_passed = 0;
    *avg = 0.0;
    *passing_avg = 0.0;

    // init ranks
    for (i = 0; i < N; i++) {
        R[i] = 1;
    }

    // compute ranks
    for (i = 0; i < N; i++) {
        for (j = 0; j < N; j++) {
            if (F[i] < F[j]) {
                R[i] += 1;
            }
        }
    }

    // compute averages
    for (i = 0; i < N; i++) {
        *avg += F[i];
        if (F[i] >= 50.0) {
            *passing_avg += F[i];
            *num_passed += 1;
        }
    }

    // check for div by 0
    if (N > 0) *avg /= N;
    if (*num_passed) *passing_avg /= *num_passed;

} // compute_ranks

I did this for the first one

or (i = 0; i < N; i += 5) {
        R[i] = 1;
        R[i+1] = 1;
        R[i+2] = 1;
        R[i+3] = 1;
        R[i+4] = 1;
    }*/

Is there a way to not use pointers or anything really to make this more optimized?

One possible optimization is to use local variables instead of directly writing to the out parameters. So instead of:

// compute averages
for (i = 0; i < N; i++) {
    *avg += F[i];
    if (F[i] >= 50.0) {
        *passing_avg += F[i];
        *num_passed += 1;
    }
}

you write:

int l_num_passed = 0;
float l_avg = 0.0;
float l_passing_avg = 0.0;

// compute averages
for (i = 0; i < N; i++) {
    l_avg += F[i];
    if (F[i] >= 50.0) {
        l_passing_avg += F[i];
        l_num_passed += 1;
    }
}

*num_passed = l_num_passed;
*avg = l_avg;
*passing_avg = l_passing_avg;

That way the compiler can use fast registers. If you directly write to the out parameter within the loop, the compiler cannot use registers as it must stay on the defensive side by assuming that avg could point to the same memory location as passing_avg or even num_passed.

This unfortunate feature of C and C++ is known as pointer aliasing and often prevents the compiler from making effective use of registers.

You can also use local variables for the other loop. Instead of:

// compute ranks
for (i = 0; i < N; i++) {
    for (j = 0; j < N; j++) {
        if (F[i] < F[j]) {
            R[i] += 1;
        }
    }
}

write:

float fi;
int rank;

// compute ranks
for (i = 0; i < N; i++) {
    fi = F[i];
    rank = 1;
    for (j = 0; j < N; j++) {
        if (fi < F[j]) {
            rank += 1;
        }
    }
    R[i] = rank;
}


Method which handle HttpResposeMessage I need to create method which handle HttpResposeMessage.
Method should throw an exception if error exist, client will handle rest.
You can see my code bellow. This code is working fine and doing what I want, but still it looks quite ugly to me.

Do you have some ideas for nicer implementation?

  [Authorize]
  public abstract class BaseController : Controller
    {
        protected readonly ILogger logger;
        protected readonly IHostingEnvironment env;
        private readonly IStringLocalizer localizer;

        public BaseController(ILogger logger, IHostingEnvironment env, IStringLocalizer localizer)
        {
            this.logger = logger;
            this.env = env;
            this.localizer = localizer;
        }

       protected void HandleResponseError(HttpResponseMessage response, ILogger logger)
        {
        if (!response.IsSuccessStatusCode)
        {
            var message = String.Empty;
            if (response.StatusCode.Equals(HttpStatusCode.Forbidden))
            {
                message = this.localizer["Forbidden"] + ": " + this.localizer["You have not the appropriate rights to access this page."];
            }
            else
            {
                try
                {
                    message = response.Content.ReadAsStringAsync().Result;
                    if (string.IsNullOrWhiteSpace(message))
                    {
                        message = message = this.localizer["Communication error with server"];
                    }
                }
                catch
                {
                    message = this.localizer["Communication error with server"];
                }
            }
            logger.LogError($"Client error message {message} !");
            throw new ApplicationException(message);
        }
    }
}

Considering that there is no else to if (!response.IsSuccessStatusCode), you should do:

if (response.IsSuccessStatusCode)
{
   return;
}

This saves on indentation and makes your method easier to read.

I'd use string.Format for things like this:

message = this.localizer["Forbidden"] + ": " + this.localizer["You have not the appropriate rights to access this page."];

Why is localizer a property on the class this method belongs to, but logger is not? And why do you need to write this.localizer?

In the end there's not much code to review, and perhaps you should consider posting the whole class.


Refactor to reduce code duplication I have four methods like these (here are only two of them):

  def checkLeft(clickedIndex: Int): Option[Int] = {
    val leftIndex = clickedIndex - 1
    if (leftIndex >= 0 && clickedIndex % field.width != 0 && isEmptyCell(leftIndex))
      Some(leftIndex)
    else
      None
  }

  def checkRight(clickedIndex: Int): Option[Int] = {
    val rightIndex = clickedIndex + 1
    if (rightIndex < field.size && clickedIndex + 1 % field.width != 0 && isEmptyCell(rightIndex))
      Some(rightIndex)
    else
      None
  }

They all have similar structures. How I can reduce code duplication here?

For the LEFT case, you're checking that an index is above the low value of zero; for the RIGHT case, you're checking that it's below the high value of field.size.  You can't really reconcile these except by combining them into a method that checks both boundaries - and there's nothing wrong with that.  Now you've got

if (inBounds(theIndex) && isEmptyCell(theIndex) && ...)
    Some(theIndex)

That ... is questionable to me because you seem to be comparing a different index (clickedIndex or clickedIndex + 1) depending on LEFT or RIGHT.

I think there are errors here that you need to reconcile before going too far down the path of duplication elimination.


Asynchronous connect() with one function call, using C++11 lambdas and Boost.Asio I've wanted to write a C++11 class for making IPv4 TCP connections and figured that it probably could be done with just one static function using some shared_ptr and lambda "magic". The following code seems to work - but I'm not entirely sure that it's safe and completely bug-free:

void Client::connect(boost::asio::io_service& io_service,
                     const std::string& address,
                     const std::string& port,
                     const Callbacks& callbacks)
{
  using namespace boost::asio::ip;

  std::shared_ptr<tcp::socket> socket = std::make_shared<tcp::socket>(io_service);
  std::shared_ptr<tcp::resolver> resolver = std::make_shared<tcp::resolver>(io_service);
  tcp::resolver::query query(address, port);

  auto connectHandler = [socket, callbacks]
                        (const boost::system::error_code& errorCode, const tcp::resolver::iterator& it)
  {
    if (errorCode) {
      callbacks.onConnectionError("Could not connect: " + errorCode.message());
    } else if (it == tcp::resolver::iterator()) {
      callbacks.onConnectionError("Could not connect!");
    } else {
      callbacks.onConnected(std::move(*socket));
    }
  };

  auto resolveHandler = [socket, resolver, callbacks, connectHandler]
                        (const boost::system::error_code& errorCode, const tcp::resolver::iterator& it)
  {
    if (errorCode) {
      callbacks.onConnectionError("Could not resolve address: " + errorCode.message());
    } else {
      boost::asio::async_connect(*socket, it, tcp::resolver::iterator(), connectHandler);
    }
  };

  resolver->async_resolve(query, resolveHandler);
}

First of all, the Callbacks is just a struct with two callbacks: onConnected(tcp::socket) and onConnectionError(const std::string&).

We'll create a tcp::socket and a tcp::resolver inside shared_ptr since they need to "survive" this whole procedure and can't be copied (atleast not tcp::socket. It can however be moved, but not using C++11 lambdas (?)).

Then we'll just create our two callbacks. resolveHandler needs to capture the socket, callbacks and connectHandler so that those "survive". It also needs to capture the actual resolver, otherwise I got an "Operation canceled"-error. The connectHandler just needs to capture the socket and the callbacks.

If the resolve is successful we'll call async_connect and tell it to try all endpoints. If the connection was successful we'll call our onConnected-callback using std::move and a reference to the socket. If it was not successful we'll call the onConnectionError-callback with an error message.

Does this looks sane? Are there any improvements I could make? Also, I'm wondering if the std::move(*socket) is safe? If I'm not mistaken, this will just move the socket to the onConnected-function call and leave the invalidated socket in the shared_ptr (which is deleted when the lambda returns). Or am I wrong?

Looks good to me.
Given what is posted there is not much else to comment about.

Object life-span: Looks good. (Though I recommend some unit-tests).
Naming: Looks good.
Style: Looks good. (Though personally I don't yet like '{' on the same line as other stuff).

The only thing I may have done differently (but this is very subjective and I would not expect you to change if you prefer your technique).

using namespace boost::asio::ip;

// I may have used
namespace BIP = boost::asio::ip;

Then prefixed all the boost stuff with BIP::. Just so I don't pollute the namespace.

But as I say your technique is totally valid since you have bound the scope of the using declaration to the function. If I was code reviewing within your company I may have commented but I would not block your merge to mainline over it.


Reversing words in a string method I tried using recursion for this.
Here is the code
  public static string ReverseWords(string str)
  {
    var splitArr = str.Split(" ");
    if(splitArr.Length == 1)
      return splitArr[0];

    return splitArr[splitArr.Length - 1] + " " + ReverseWords(String.Join(" ", splitArr.Take(splitArr.Length - 1));
  }

This is horribly inefficient both in terms of storage and runtime:

As already mentioned in comments (though not spelled out) recursion has a memory overhead directly proportional to the number of iterations - which in you case is input size.

Remember - every function call takes up stack space, and a recursive call is the same as regular call!

You create extra temporaries by rejoining the string you just split for the recursive call and having a string concatenation on each iteration.

That's two new strings create in every step in addition to the split array!

Everything mentioned in step 2 takes processing time as well as memory.

You will not notice this overhead with any string you might type in a console for testing, but try reading and reversing a sizable text file, and the problem will become apparent.