Pattern for writing a generic string transformation function I often need to write string processing functions that take an input string and transform that into some output string. I'm looking for a pattern to make such functions as generic as possible (with reasonable effort).

Requirements

It should be possible to supply the input as a range (like std::string or std::string_view), pair of iterators, null-terminated character arrays and pointers.
Character type should not be restricted to char, but also support wchar_t. Support char16_t and char32_t only if it doesn't require additional effort as I currently have no use for them.
Easy to use for the simple case where input and output is a std::basic_string.
Easy to write and more importantly easy to read.
Performance is less important. Processing character-by-character is good enough.

I currently approach this by splitting the interface into two template functions. The first one is the idiomatic generic interface which is iterator based, the second one is range based for convenience. The convenience interface is allowed to trade some generalism for ease-of-use.

Example

Escape all occurences of the characters '\n', '\t' and '\\' in the input string.

First the idiomatic interface.

template< class InputIt, class OutputIt >
OutputIt EscapeString( InputIt first, InputIt last, OutputIt out )
{
    while( first != last )
    {
        auto c = *first++;
        switch( c )
        {
            case '\n':
                *out++ = '\\';
                *out++ = 'n';
                break;
            case '\t':
                *out++ = '\\';
                *out++ = 't';
                break;
            case '\\':
                *out++ = '\\';
                *out++ = '\\';
                break;
            default:
                *out++ = c;
        }
    }
    return out;
}

Now the convenience wrapper. InputString can be any string range (like std::string), null-terminated char array or pointer to null-terminated string.
The result will be std::basic_string<character_type_of_InputString> by default, but can be changed to any class that has a push_back() member.

template< typename InputString,
          class ResultString = typename as_literal_traits<InputString>::std_string_type >
ResultString EscapeStringCopy( const InputString& input )
{
    // Allow both the ADL-selected overloads for user-defined types and the standard
    // library function templates to appear in the same overload set.
    using std::begin; using std::end;

    // Handle null-terminated character arrays, character pointers and ranges uniformly.
    auto inputRange = boost::as_literal( input );

    ResultString result;
    EscapeString( begin( inputRange ), end( inputRange ), back_inserter( result ) );
    return result;
}

The template as_literal_traits is an utility that is used to create a default return type of std::basic_string that has the same character type as the input string.

template< typename StringT >
struct as_literal_traits
{
    // NOTE: std::declval<StringT const&>() is used instead of just StringT{}
    // in case StringT is not default-constructible.
    using range_type = decltype( boost::as_literal( std::declval<StringT const&>() ) );
    using value_type = typename boost::range_value< range_type >::type;
    using std_string_type = std::basic_string< value_type >;
};

Live demo at Coliru

Any improvements are welcome.

Look at Boost.Range.  Your first point drives most of the rest of the concerns.

Write your template function to take a Range.  You can pass it a C string literal, a std::string, an array, vector, or anything —— even a pair of iterators if that’s what you need after all.

Not limited to char?  No problem: make it a Range of T, with a default of char.

This takes the place of your initial idiomatic approach.  Use ranges rather than iterators, and you don’t need to wrap it for convenience.  Boost.Range does all that for you.

The output can remain an output iterator, but I like to make it a range adaptor.

With that, you can do things like:

for (auto c : mystring|escaped) { ⋯


Implementing the midpoint displacement algorithm in Java Thanks for taking the time to read this first off. I want to say that I have always been very interested in generating random terrain and since I'm new to any sort of graphical programming, I decided to start as simple as I could get.

I am trying to implement the midpoint displacement algorithm to make fractal type mountains. I've read the most common resources on it and understand the concept but that hasn't made it any easier. I suppose I have some more general questions about how I would store data effectively, but this is the code I have come up with. The code is pretty awful but it produces numbers and I've tried to make lines with the different points in openGL but I'm getting some pretty crazy results. I'm using the idea discussed on the start of this page. http://www.gameprogrammer.com/fractal.html

import java.util.Random;

public class MidPointGenerator {

private static double[] heightmap;
private static double randrange;
private static int iterations;
private static double roughness;
private static int n;

public MidPointGenerator(double s, int i, double h){
    n = (int)(Math.pow(2, i) + 1); // size of the array
    heightmap = new double[(int)n];
    randrange = s;
    iterations = i;
    roughness = Math.pow(2,-h);
    midPoint(0, n-1, randrange,0,i);
}

public static void midPoint(int left, int right, double range, int i, int goal){

    if(i == goal){
        return;
    }

    int mp = (left + right) / 2;
    double mpheight;

        if(heightmap[left] < heightmap[right]){
        mpheight = ((heightmap[right] - heightmap[left]) / 2.0) + heightmap[left];
        heightmap[mp] = mpheight + random(range);
    }
    if(heightmap[right] < heightmap[left]){
        mpheight = ((heightmap[left] - heightmap[right]) / 2.0) + heightmap[right];
        heightmap[mp] = mpheight + random(range);
    }
    if(heightmap[right] == heightmap[left]){
        heightmap[mp] += random(range);

    midPoint(left, mp, range * roughness, i+1, goal);
    midPoint(mp, right, range * roughness, i+1, goal);

}

public static double random(double range){
    //Returns a random number from -s to s.
    Random random = new Random();
    double randNum =  range - ((2*range) * random.nextDouble());
    return randNum;
}

public static void displayHeights(){
    for(int i = 0; i < heightmap.length; i++){
        System.out.println(heightmap[i]);
    }
}

public double getHeightValue(int index){
    if(index == n ){
        return 0;
    }
    return heightmap[index];
}

public int getIterations(){
    return n;
}

}

So this at least runs, stores the height values in a 1D array thats size depends on the number of iterations I want to do. How can I make my code much more logical and perhaps there is a much better way of implementing this. I was more concerned about just getting something that compiled and ran successfully. I'm convinced the way I'm approaching this is flawed and there is a much better way.

Any criticism is welcomed!
Thanks,
Ryan

.... Hrm...

import java.util.Random;

I'm assuming this is sufficient for your needs.  In the future, it may be preferable to create your own interface, that you can supply different implementations for.

public class MidPointGenerator {

    private static double[] heightmap;
    private static double randrange;
    private static int iterations;
    private static double roughness;
    private static int n;

What does n represent?  Most of the other variables are named okay (although randrange should probably be deviation, or similar).
Mutable class-level variables should (almost always) die.  Especially since:

public MidPointGenerator(double s, int i, double h){
    n = (int)(Math.pow(2, i) + 1); // size of the array
    heightmap = new double[(int)n];
    randrange = s;
    iterations = i;
    roughness = Math.pow(2,-h);
    midPoint(0, n-1, randrange,0,i);
}

You set them in the instance constructor!  So, they're really instance variables!  If I were attempting to use your class, I might end up mighty confused as to behavior, when attempting to use two different instances.  Also, your parameter names are terrible - they don't tell me at all what they're used for.  And don't use end-of-line comments.

public static void midPoint(int left, int right, double range, int i, int goal){

    if(i == goal){
        return;
    }

What does i represent?  The only time such short variables are ever really acceptable is as indices during iterators (maybe).  This method is static because it's operating on class-level variables (which should be instance-level), but there's no good reason for it to be flagged public as well.

    int mp = (left + right) / 2;
    double mpheight;

    if(heightmap[left] < heightmap[right]){
        mpheight = ((heightmap[right] - heightmap[left]) / 2.0) + heightmap[left];
        heightmap[mp] = mpheight + random(range);
    }
    if(heightmap[right] < heightmap[left]){
        mpheight = ((heightmap[left] - heightmap[right]) / 2.0) + heightmap[right];
        heightmap[mp] = mpheight + random(range);
    }
    if(heightmap[right] == heightmap[left]){
        heightmap[mp] += random(range);

    }  // <- Missing?

I'm assuming, given other indentation, that the brace (}) belongs here.  Compiler error otherwise.

    midPoint(left, mp, range * roughness, i+1, goal);
    midPoint(mp, right, range * roughness, i+1, goal);

}

public static double random(double range){
    //Returns a random number from -s to s.
    Random random = new Random();

You're creating a new Random object each time you enter this object (and not using the variable you created earlier...).  Depending on how your program runs, you may have multiple instances of Random with the same seed value, causing them to generate the same output; while occasionally random generators should return the same output, getting them in this fashion is undesirable.  Same comments about public and static.

    double randNum =  range - ((2*range) * random.nextDouble());
    return randNum;
}

random.nextDouble() returns an evenly distributed value in the range [0, 1).  The result of the math, though, is not evenly distributed along (-range, range); at least, I don't think so (anybody else know better?  help?).  You can generate double-ended integer ranges (with Random.nextInteger(upper)), which doesn't really help here.  You may need a new random generator, if you're going to get 'correct' results - what you have may be good enough for your current purposes.

public static void displayHeights(){
    for(int i = 0; i < heightmap.length; i++){
        System.out.println(heightmap[i]);
    }
}

Same comments about public and static.  Really, you should be using a for-each loop.  Oh, and not printing out to the console directly, but that's probably forgivable here.

public double getHeightValue(int index){
    if(index == n ){
        return 0;
    }
    return heightmap[index];
}

public int getIterations(){
    return n;
}

I'm assuming that these methods are for whatever needs to inspect the state of the map after you're done constructing it (hence the public), given it's not used anywhere in your code.  You really ought to be returning some sort of array/collection, though (probably a copy of heightmap), which should be easier to work with.

Ah, I think this is more along the lines of how I would have done it:

public class MidPoint {

    private static final Random RANDOM = new Random();

    private final double[] heightmap;

    private MidPoint(double[] heightmap) {
        this.heightmap = heightmap;
    }

    private double[] getHeightmap() {
        return Arrays.copyOf(heightmap, heightmap.length);
    }
}

This is the basic class definition (static creation methods in a second).  Because the class now represents the end state, I felt it was appropriate to change the name.  The constructor is private, because it doesn't make much sense for it to be manually created.  And the end heightmap should be accessed as a copy, so that the result can be safely shared.  It's also being output as the array, not as by-index, which you may find easier to work with.

private static double random(double range) {
    return range - ((2 * range) * RANDOM.nextDouble());
}

I just tightened this up a little, was all.  It might not be 'perfect', but I personally don't know of a better way to set this up, if the variation is an issue.  Actually, this method makes (temporary) testing really easy - just have it return a constant value!

private static double getMidpoint(double a, double b) {
    return ((a - b) / 2) + b;
}

Helper method, to remove code duplication.

private static double getOffset(double current, double left, double right) {
    final int comparison = Double.compare(left, right);

    if (comparison < 0) {
        return getMidpoint(left, right);
    } else if (comparison > 0) {
        return getMidpoint(right, left);
    } else {
        return current;
    }
}

... Mathematically speaking, the greater/less than shouldn't be necessary (because the midpoint calculation gives correct results either way).  The problem comes in that the way doubles (and limited-precision numbers of all types) are represented won't necessarily always give the same results.

private static void midPoint(double[] heightmap, int iteration, int limit, int left, int right, double range, double deviation) {
    if (iteration == limit) {
        return;
    }

    final int midpoint = ((right - left) / 2) + left;

    heightmap[midpoint] = (getOffset(heightmap[midpoint], heightmap[left], heightmap[right]) + random(range));

    final double adjusted = range * deviation;

    midPoint(heightmap, iteration + 1, limit, left, midpoint, adjusted, deviation);
    midPoint(heightmap, iteration + 1, limit, midpoint, right, adjusted, deviation);
}

Recursive method.  Things have mostly just been extracted to helper methods.  The only big difference is the way the midpoint is retrieved, which was changed so that with larger iterations (because of the squaring from iterations) they won't overflow.

private static double[] getMidPoints(final int iterations, final double range, final double roughness) {
    final double[] heightmap = new double[(int) Math.pow(2, iterations) + 1];
    final double deviation = Math.pow(2.0, roughness);

    midPoint(heightmap, 0, iterations, 0, heightmap.length - 1, range, deviation);

    return heightmap;
}

Setup for entire process, returning the finished array.

public static MidPoint generate(final int iterations, final double range, final double roughness) {

    if (iterations >= 31) {
        throw new IllegalArgumentException("Too many iterations, must be 30 or less");
    }

    if (Double.isNaN(range) || Double.isInfinite(range) || !(range > 0.0)) {
        throw new IllegalArgumentException("Range must be a positive value greater than 0");
    }

    if (Double.isNaN(roughness) || Double.isInfinite(roughness) || !(roughness > 0.0)) {
        throw new IllegalArgumentException("Roughness must be a positive value greater than 0");
    }

    return new MidPoint(getMidPoints(iterations, range, roughness));
}

And... The static generator method, returning the final value object.  The biggest thing here was the error checking.

Oh, the results always have 0.0 at the start and end of the set of midpoints, which makes me think something is up, but I have no idea what.  Unfortunately, I don't know what you're trying to implement, so I can't tell if something is wrong.  And no, it's not something I introduced - yours gets that too (constant values for the random get the same results).


Dice simulator program I recently began to dabble into the programming world and wrote a Dice Simulator Program. How do I know that I'm writing Good Code? In the sense, is it something similar to how a major Software Developing company would produce code for real problems.

""" This Program is Dice Simulator that produce an Output from 1-6
everytime the User presses Enter. In order to close the Program,
the User has to type "Exit" into the console.
"""

#Imports the random module to generate the random Integers
import random

#Initial Designer Print Statements
print(" Hi! Welcome to Dicey!")
print ("""

   .-------.    _______
  /   o   /|   /\\      \\
 /_______/o|  /o \\  o   \\
 | o     | | /   o\\______\\
 |   o   |o/ \\o   /o     /
 |     o |/   \\ o/  o   /
 '-------'     \\/____o_/

  """)

exit=False      #Set True When User types "Exit" into the console
count=0         #Count to keep track of Iterations/Dice rolls

#The Input is obtained below
enter=(input(("Press Enter to Roll (or) Type 'Exit' to Close.
\n\n")).lower())

#Condition to check wether the user wants to Exit the program without
#Rolling the Dice

if (enter=="exit"):
    exit=True

#Checks for Enter and Produces a random integer using the random module
while(enter=="" or not exit):
  dice_number=random.randint(1,6)
  count = count + 1
  print(f"{count}>You got a {dice_number}")
  enter=(input("Press Enter to Roll Again! \n").lower())

#If User inputs "Exit", Close the program.
if (enter=="exit"):
    exit=True

#If the User inputs anything else other than "Exit" Roll the dice again.
elif(enter is not ""):
    print("\n")
    continue

Here are a few suggestions on how to simplify and format your code.
Break directly from the loop
In your loop construct there really is no point in keeping track of exit.  In addition exit is used by Python, and it is better to not reuse these things becuase it can be confusing.  Instead something like:
while True:
    if input(msg).lower() == 'exit':
        break

In addition this shows how you really only need one input statement.  Instead the message for input can be set as needed.
Avoid extra parens
In all three of these cases you have an extra set of outer parens.
if (enter=="exit"):
while(enter=="" or not exit):
enter=(input("Press Enter to Roll Again! \n").lower())

Use descriptive variable names
As an example you have a variable named count, that I think would be more descriptive as roll_number.  Additionally dice_number might be more descriptive as dice_value.
Use the programming language to its fullest.
Python has a bunch of nifty operators that can make code cleaner.
count = count + 1

Can be written as:
count += 1

which makes it more explicit that the variable is being incremented, and not just assigned.
PEP8
You should consider formatting your code in accordance with pep8.  This is important when sharing code, as the consistent style makes it much easier for other programmers to read your code.  There are various tools available to assist in making the code pep8 compliant.  I use the PyCharm IDE which will show pep8 violations right in the editor.
One PEP8 note that I will highlight is to not use what are relatively obvious comments.  With clean code and descriptive variable names the code will often be as descriptive as the comments.
Reconstructed code:
""" This Program is Dice Simulator that produce an Output from 1-6
everytime the User presses Enter. In order to close the Program,
the User has to type "Exit" into the console.
"""

import random

print(" Hi! Welcome to Dicey!")
print("""

   .-------.    _______
  /   o   /|   /\\      \\
 /_______/o|  /o \\  o   \\
 | o     | | /   o\\______\\
 |   o   |o/ \\o   /o     /
 |     o |/   \\ o/  o   /
 '-------'     \\/____o_/

  """)

roll_number = 0
msg = "Press Enter to Roll (or) Type 'Exit' to Close.\n\n"
while True:
    if input(msg).lower() == 'exit':
        break

    roll_number += 1
    dice_value = random.randint(1, 6)
    print(f"{roll_number}> You got a {dice_value}\n")

    msg = "Press Enter to Roll Again! \n"


"Guess my Number" game in Python (2.7.9) First time posting to Code Review. Just looking to get some advice on how I can write better code.

I found the following exercise for a "Guess my Number" game on DaniWeb, but the linked source code file is no longer active. So while I know my code works, I'm not sure if I've written it in the best way.

  "The computer randomly generates a number. The user inputs a number,
  and the computer will tell you if you are too high, or too low. Then
  you will get to keep guessing until you guess the number."

(I also added a limited number of tries to the game, so the user can't try 1-10 in order until they guess right.)

from random import randint

### Variable declarations
correct = False
tries = 0
number = randint(1,10)
###

print ("I'm thinking of a number between 1 and 10.")
print ("I'll give you 5 chances to guess it.\n")

# Ask the user for their guess, as long as there are tries remaining
# and they haven't already guessed correctly.
while correct == False and tries < 5:

    guess = raw_input("What number am I thinking of?: ")

    # Make sure user's input is an integer
    try:
        guess = int(guess)

        if guess == number: # Correct answer
            correct = True
            print ("\nYou got it!")

        elif guess > number:    # Guess too high
            if guess > 10:
                tries += 1
                print ("\nWay too high! Try a number 10 or lower.")
            else:
                tries += 1
                print ("\nTry something lower.")

            remaining = 5 - tries
            print ("\nYou have " + str(remaining) + " guesses left.\n")

        else:
            if number > guess > 0:    # Guess too low
                tries += 1
                print ("\nTry something higher.")
            else:
                tries += 1
                print ("\nMake sure you guess a number 1 or higher!")

            remaining = 5 - tries
            print ("\nYou have " + str(remaining) + " guesses left.\n")

    # Input not an integer
    except ValueError:
        print ("\nThat wasn't a number! Try again.")
        print ("(I won't count that attempt.)")

        remaining = 5 - tries
        print ("\nYou have " + str(remaining) + " guesses left.\n")

### Game over! ###

# Ran out of guesses
if tries > 5:
    print ("\nSorry, you ran out of guesses!")
    print ("My number was " + str(number) + ".")

# Exited loop with tries left, must have guessed correctly
else:
    print ("Well done. Thanks for the game.")

I'd love to get some feedback on what I have here. Thanks in advance!

Magic numbers

1, 10, and 5 are so-called magic numbers in the code: they are duplicated in many places. The numbers themselves don't carry a meaning, but they are special to the program. If you wanted to change the maximum number of tries, you would have to manually search and replace all occurrences of 5. If the number 5 is used for other than the purpose of maximum number of tries, then the change will be especially difficult, as you would have to review each occurrence to verify its purpose.

The solution is simple: give the magic numbers meaningful names, for example:

lower_limit = 1
upper_limit = 10
max_tries = 5

You might even want to make these constants by uppercasing the names.

Copy-paste coding

This piece of code appears multiple times:

remaining = 5 - tries
print("\nYou have " + str(remaining) + " guesses left.\n")

Most probably you copy-pasted it every time you needed.
Try to avoid pressing ControlV while programming.
Try solution is almost always to create a new function, for example:

def print_remaining_guesses(tries):
    remaining = max_tries - tries
    print("\nYou have " + str(remaining) + " guesses left.\n")

Use "{}".format(...) for formatting

Instead of:

print("\nYou have " + str(remaining) + " guesses left.\n")

The recommended way is:

print("\nYou have {} guesses left.\n".format(remaining))

This is a kind of templating. By removing + varname + from the middle of strings, the text becomes a bit easier to read. Another benefit is that you can drop the str(...) wrapper, format will automatically take care of that for you.

Pull common operations up

Looking at this code:

if guess > 10:
    tries += 1
    print("\nWay too high! Try a number 10 or lower.")
else:
    tries += 1
    print("\nTry something lower.")

The value of tries will be increment no matter what. So you can pull it up, out of the if-else:

tries += 1
if guess > 10:
    print("\nWay too high! Try a number 10 or lower.")
else:
    print("\nTry something lower.")

Keep try blocks small

You have a very large try block here:

try:
    guess = int(guess)

    # ... many lines ...

# Input not an integer
except ValueError:
    # ...

Since the only place where you expect something to go wrong is the guess = int(guess) line, it would be better to wrap only that in the try-except.

Comparison with boolean values

Don't use == or != with boolean values. Instead of:

while correct == False:

The recommended way is:

while not correct:

Pointless comments

The comments on these lines really don't tell anything new:

if guess == number:  # Correct answer
    # ...

elif guess > number:  # Guess too high

Let the code speak for itself. Then you really don't need comments at all.

Formatting

The space between print and ( is redundant, you should remove it:

print ("I'm thinking of a number between 1 and 10.")

Python has an official style guide called PEP8, I suggest to review it and follow it.


Extract rotation values from css string Javascript I have the following function that takes out the rotation values from a css string and returns them as an array.

var extractRotationValues = function(code) {
  var getAllIndexes = function(arr, val) {
    var indexes = [],
      i = -1;
    //To understand the second parameter of indexOf function
    //have a look at this : https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/indexOf
    while ((i = arr.indexOf(val, i + 1)) != -1) {
      indexes.push(i);
    }
    return indexes;
  };
  var indexes = [];
  var closingIndexes = getAllIndexes(code, ")")
  var values = [];
  var rindexes = [];
  rindexes.push(code.indexOf("X("), code.indexOf("Y("), code.indexOf("Z("));
  //No value for rotateX was found so consider it 0
  if (rindexes[0] == -1)
    values.unshift(0);
  else {
    //We have a value for rotateX
    //Where does rotateX start from => rindexes[0]
    //Where does it end? =>closingIndexes[0]
    var value = code.substr(rindexes[0], closingIndexes[0]).split("(")[1].split("deg)")[0];
    values.push(value)
  }
  //No value for rotateY
  if (rindexes[1] == -1)
    values.splice(1, 0, 0);
  else {
    //We have a value for rotateY
    //Where does rotateY start from => rindexes[1]
    //Where does it end? =>closingIndexes[1]
    values.push(code.substr(rindexes[1], closingIndexes[1]).split("(")[1].split("deg)")[0])
  }
  //No value for rotateZ
  if (rindexes[2] == -1)
    values.splice(2, 0, 0);
  else {
    //We have a value for rotateZ
    //Where does rotateZ start from => rindexes[2]
    //Where does it end? =>closingIndexes[2]
    var zvalue = code.substr(rindexes[2], closingIndexes[2]).split("(")[1].split("deg)")[0]
    values.push(zvalue);
  }
  return values;
};

console.log(extractRotationValues("rotateX(90deg) rotateY(87deg) rotateZ(22deg)"))

I was wondering.Is there anything that can be done in a better way here or something to improve?

First: A little regexp will do ya just as well, I'd imagine. Something like:

function extractRotationValues(string) {
  var values = {x: 0, y: 0, z: 0};
  string.replace(/\brotate([XYZ])\s*\(\s*([+-]?[\d.]+)\s*deg\s*\)/g, function (_, axis, value) {
    values[axis.toLowerCase()] = parseFloat(value);
  });
  return values;
}

Note that this does a couple of things differently:

parses the string values into Number values
returns an object with x, y, and z properties rather than an array (just because I feel that that's more useful - it can be made to return an array, though)

Basically it looks for rotate[name of axis]([a number]deg) (while skipping over possible whitespace - the \s* stuff - that might be in the string) and captures the axis name and number. Despite the use of replace nothing's actually replaced (or rather we don't care about replacing things). It's just that String.replace takes a function as its 2nd argument, which gets passed the matched/captured substrings, which sort of turns replace into a "scan"-like function.

Anyway, that's what I'd do. Not saying it's bulletproof (spent a minute writing it, and robustly matching a number can be more complicated), but it's certainly less code.

Your code works too, of course, though the lower-level approach of finding indices and slicing and dicing strings is less elegant in my eyes. But it's a valid approach nonetheless.

There are however a few things I want to comment on:

I'd encourage you to always use strict equals (=== and !==). Just because it's a good habit to get into.
closingIndexes is a nice descriptive name by itself, but it gets confusing when you also have rindexes where you've abbreviated, and just plain indexes (which isn't actually used). I'd give all of them equally descriptive names, instead of having one that's descriptive, one that's slightly cryptic, and one that's generic. (I also tend to favor indices for the plural form, but "indexes" is correct English too.)
Why are default zero values inserted into the array in a different way than extracted/found values? The found values you just insert with push, which makes a lot of sense. But when you want to insert 0 for x, y, or z, you use unshift or splice? Why not use push in all cases, as that's the simplest?
I'd rather have a branch that determines the value to insert, and a common push(value) call after that, than branch the array-insertion itself, because in both cases you want to append some value. The branching should be about what to insert, because how and where to insert it is always the same.
Incidentally, if you find a value, you insert it as a string. If you don't find one, you insert zero as a number. Don't mix types like that; keep it consistent. If you'd used null or undefined instead of zero to denote "no value", that'd be fine, but here you're essentially "making up" a rotation that's not present in the CSS string, so it's not "no value", it's "fallback value". And thus it should be the same type as a non-fallback value and vice-versa.
Your code can stumble on malformed CSS. E.g. rotateX(42deg rotateY(23deg) (note the missing closing parenthesis) won't be parsed right. It'll find 42deg rotateY as the value for x-axis rotation, which you might say is better than nothing. However, the CSS was malformed, so technically speaking there is no x-axis rotation; nothing would actually be better.
Most importantly: Why write 3 separate if-else blocks when all that changes in them is an index? That's what loops are for. Write a regular for loop, or call forEach on rindexes, e.g.:

rindexes.forEach(function (rindex, i) {
  var value = 0;
  if(rindex !== -1) {
    value = code.substr(rindex, closingIndexes[i]).split("(")[1].split("deg)")[0];
  }
  values.push(value);
});


Calculate median of a matrix I am solving interview questions from here.

  Problem : Given a N cross M matrix in which each row is sorted, find the overall median of the matrix. Assume N*M is odd.

  Note: No extra memory is allowed.
  For example:

 Matrix=     [1, 3, 5]
             [2, 6, 9]
             [3, 6, 9]

  A = [1, 2, 3, 3, 5, 6, 6, 9, 9]
  Median is 5. So, output is 5.

This is my approach:

def find_median( A):
    """Returns the median value from given list"""
    for i in range(1,len(A)):
        A[0].extend(A[i])

    return (sorted(A[0])).pop(len(A[0])/2)

Test Cases:

assert find_median([[1,3,5],[2,5,9],[3,6,11]]) == 5
assert find_median([[0,1,1],[2,6,10],[3,5,9]]) == 3
assert find_median([[1,3,4,12,14],[1,6,9,10,15],[0,1,3,3,4]]) == 4

I am able to solve the problem but I wanted to know is there a better approach to solve this problem?

Follow PEP8

A is a bad variable name, use say matrix.
You should remove the space in-front of the function argument.
You should put spaces after ,.
You don't need the () surrounding sorted.
You could add some space around your division.

You can use // rather than / to make your code Python 2 and Python 3 compatable.
You don't need to use pop, normal indexing will work too.

def find_median(matrix):
    """Returns the median value from given matrix"""
    for i in range(1, len(matrix)):
        matrix[0].extend(matrix[i])
    return sorted(matrix[0])[len(matrix[0]) // 2]

Your code doesn't work as the challenge asks, if you add print(matrix) before return you'll see:

[[1, 3, 5, 2, 6, 9, 3, 6, 9], [2, 6, 9], [3, 6, 9]]


Convert Integer to Date in Java Due to some clueless soul, we have several fields in a legacy database that uses a 6-digit integer to represent a date in (YYYYmm) format. The date field is assumed to be the last day of the given month.

I have written the following method to perform the conversion from int to java.util.Date:

public Date convertIntToDate(Integer intDate) {

    if (intDate < 100000 || intDate > 999999) {
        log.warn("Unable to parse int date {}", intDate);
        return null;
    }

    int intYear = intDate/100;
    int intMonth = intDate - (intYear * 100);

    Calendar result = new GregorianCalendar();
    result.set(intYear, intMonth - 1, 1, 0, 0, 0);

    return result.getTime();
}

It seems to work, but if anyone has any suggested improvements, I'd love to hear them.

The date field is assumed to be the last day of the given month.

This seems to suggest that you would want to convert 201502 to 2015 Feb 28,
as that's the last day of 2015 Feb.
Your code converts to 2015 Feb 01, I hope that's what you really meant.

Your validation is too weak:

if (intDate < 100000 || intDate > 999999) {
    log.warn("Unable to parse int date {}", intDate);
    return null;
}

So, 999999 is a valid date? The method will convert that to 10007-03-01.

What about 201599? The method will convert that to 2023-03-01. Not so great.

I'd suggest to do the validation after converting to intYear and intMonth.
It will be easier to follow and more logical.

Why does the method take an Integer?
It doesn't make sense, and it makes the method prone to NullPointerException.
(The code will throw when reaching intDate < 100000 in the if condition.)

The conversion to month would be more readable with a modulo:

    int intMonth = intDate % 100;

And the way you set the month is incorrect,
as the second value of result.set is supposed to be a constant like Calendar.JANUARY.
The correct way:

    result.set(Calendar.YEAR, intYear);
    result.set(Calendar.MONTH, intMonth - 1);
    result.set(Calendar.DAY_OF_MONTH, 1);


Splitting three comma-delimited strings I've been reading about loops this morning, to try to improve this snippet of code:

$deliveryoutcode1 = array();
$deliveryoutcode1 = explode(",",$rows['deliveryoutcode1']);
$deliveryoutcode2 = array();
$deliveryoutcode2 = explode(",",$rows['deliveryoutcode2']);
$deliveryoutcode3 = array();
$deliveryoutcode3 = explode(",",$rows['deliveryoutcode3']);

I started to read about loops this morning and I came up with this:

$i=1;
while ($i <= 3) { // Output values from 0 to 3
    $deliveryoutcode[$i] = array();
    $deliveryoutcode[$i] = explode(",",$rows['deliveryoutcode' . $i . '']);
    $i++;
}

This works great, but I'm wondering if there is a better way to do this. It appears as though I've come up with the best way but as this is my first attempt at loops, I better ask the experts.

Several things to mention:

1. Use a for loop.

Semantically speaking, a for loop is way more suited in your case than a while loop. for usually means "repeat this for a specified number of steps" while while means "repeat this as long as a given condition is true". Given, you can ALWAYS replace a for with a while from a technical point of view and exactly this is the reason why you need to decide which one to use based on semantics

2. No need to use explicit array initialization.

When you assign the result of explode to your array object the interpretor will take care of the object type. There is no reason in this case to make it explicit so it is better to delete that line.

3. Naming

Not sure if this is part of the exercise or not, but consider using proper names for your objects. What do you explode? What does the array contains? Is a list of houses? Are those candies? Users? deliveryoutcode does not have too much meaning.

If you really don't have any real objects to work on, invent some and use those. It creates good habits for your programming journey.


Design for C++ class methods taking input from different sources I have a class foo that needs to get input from different sources, namely input streams,
arrays and iterators. The code that actually does the work in the end takes iterators as
input, and all the other methods end up calling it behind the scenes:

void from_stream(std::istream& in); // Read from stream and insert at end
void from_stream(std::istream& in, iterator it); // Read from stream and insert at it

template<typename T>
void from_buffer(T* buffer, size_t len); // Read from a buffer and insert at end
template<typename T>
void from_buffer(T* buffer, size_t len, iterator it); // Read from a buffer and insert at it

// The work is done here
template<typename InputIterator>
void decode(InputIterator first, InputIterator last); // Decode range and insert at end
template<typename InputIterator>
void decode(InputIterator first, InputIterator last, iterator it); // Decode range and insert at it

I want users to be able to make calls like these (assume foo class is initially empty):

Foo foo;
foo.from_stream(my_stream, foo.begin()); // or without foo.begin()
foo.from_buffer(my_buffer, buflen, foo.begin() + 4); // same...

and internally the code will call specialized std::inserter, std::front_inserter
or std::back_inserter functions as appropriate and pass them to foo::decode. I could just use
std::inserter all the time, but I would think it is more efficient to use std::back_inserter
when foo.end() is passed and so on.

I could change the iterator argument to a template parameter and let the user decide
what kind of iterator to use similar to the STL (which is actually what I am trying to aim for). This would also allow the user to pass a foo::iterator and overwrite content instead of inserting data. This would change the calls above to:

foo.from_stream(my_stream, std::back_inserter(foo));
foo.from_buffer(my_buffer, buflen, std::inserter(foo, foo.begin() + 4));

I could use some design advice about what approach, or another one entirely, is "best".

Why not just insert() and iterator like most containers in the STL.

 std::vector<int>   data;
 std::ifstream      file("Plop");

 data.insert(data.end(), std::istream_iterator<int>(file),
                         std::istream_iterator<int>());

 char  buffer[] = "1 2 3 4 5 6 6 7 8 9 10 11 12 13 14 15 16 17 18 19";
 std::stringstream   bufferStream(buffer);

 data.insert(data.end(), std::istream_iterator<int>(bufferStream),
                          std::istream_iterator<int>());

 std::vector<int>    anotherBuffer = LoadBuffer();
 data.insert(data.end(), std::begin(anotherBuffer), std::end(anotherBuffer));


Steam Community Market Strange Part Scraper A while back I wrote a simple little PHP script that searches the Steam community market for any TF2 strange weapons with strange parts on the first page of results for that weapon type. It works by retrieving the listings for each weapon using curl, runs a regex to retrieve the internal JS variable containing the item metadata, and parses the JSON and looks in the descriptions for any strange part strings:

#!/usr/bin/php

<?php

# list of weapon types to look for

$weapons = array(
  0 => "Scattergun",
  1 => "Pistol",
  2 => "Rocket%20Launcher",
  3 => "Direct%20Hit",
  4 => "Shotgun",
  5 => "Flame%20Thrower",
  6 => "Flare%20Gun",
  7 => "Reserve%20Shooter",
  8 => "Axtinguisher",
  9 => "Grenade%20Launcher",
  10 => "Loch-n-Load",
  11 => "Stickybomb%20Launcher",
  12 => "Eyelander",
  13 => "Minigun",
  14 => "Tomislav",
  15 => "Dalokohs%20Bar",
  16 => "Killing%20Gloves%20of%20Boxing",
  17 => "Pomson%206000",
  18 => "Widowmaker",
  19 => "Rescue%20Ranger",
  20 => "Frontier%20Justice",
  21 => "Wrangler",
  22 => "Sniper%20Rifle",
  23 => "Bazaar%20Bargain",
  24 => "SMG",
  25 => "Jarate",
  26 => "Bushwacka",
  27 => "Revolver",
  28 => "Ambassador",
  29 => "Knife",
);

for($wep_counter = 0; $wep_counter < count($weapons); $wep_counter++) {

  $url = "http://steamcommunity.com/market/listings/440/Strange%20". $weapons[$wep_counter];       # get listing for that weapon type

  $handle = curl_init($url);
  curl_setopt($handle, CURLOPT_RETURNTRANSFER, TRUE);

  $resultraw = curl_exec($handle);

  preg_match("/var g_rgAssets = (\{.*\});/", $resultraw, $resultsearch);                           # retrieve the JSON item metadata from the page

  $json_string = rtrim($resultsearch[1]);
  $json_data = json_decode($json_string,true);

  for($count = 0; $count < count($json_data["440"]["2"]); $count++) {                              # iterate through each item
    $item = array_values($json_data["440"]["2"])[$count];
    if(array_key_exists("descriptions",$item)) {                                                   # check for an item description; if it has a strange part it'll be listed in the description
      for($descriptions = 0; $descriptions < count($item["descriptions"]); $descriptions++) {
        $description = array_values($item["descriptions"])[$descriptions];
        if(array_key_exists("color", $description) && $description["color"] == "756b5e") {         # if any of the description strings have the color 756b5e (color of the strange part indicator)...
          print $url . ", ";                                                                       # output what item it is and the index on the page for easy retrieval
          print $count + 1 . "\n";
        }
      }
    }
  }

  print "Sleeping" . "\n";                                                                         # sleep for 15 seconds to avoid rate limiting
  sleep(15);
}

?>

I'd like to get some feedback on how I can make it cleaner/clearer/better. In particular, the big $weapons array at the beginning is super ugly; is there a way I could have that be more compact? Perhaps something with an explode?

Not exactly a PHP expert but your $weapons array:

$weapons = array(
  0 => "Scattergun",
  1 => "Pistol",
  2 => "Rocket%20Launcher",
  3 => "Direct%20Hit",
  .
  .
  .
  29 => "Knife",
);

You do not need an associative array for this. In fact, I would discourage this. Although there may be no good reason to do this, imagine you want to add a weapon in the middle, then you have to change a lot of numbers.

Similarly, I do not know how PHP internally represents associative arrays, but they may be slower to access than a standard array. C++ implementations often use Red-Black Trees to implement something similar to associative arrays and other languages may use other data structures.

Just use the standard array:

$weapons = array(
  "Scattergun",
  "Pistol",
  "Rocket%20Launcher",
  "Direct%20Hit",
   .
   .
   .
  "Knife",
);

Use a foreach loop:

for($wep_counter = 0; $wep_counter < count($weapons); $wep_counter++) {

  $url = "http://steamcommunity.com/market/listings/440/Strange%20". $weapons[$wep_counter];       # get listing for that weapon type

becomes:

foreach ($weapons as $weapon) { # I don't believe you are mutating a value so I don't believe the "&" prefix is necessary.

  $url = "http://steamcommunity.com/market/listings/440/Strange%20". $weapon;

There could be other issues but I'm not too much of a PHP expert, but these issues pertain to a lot of languages. I would definitely be sure to keep these in mind when using other languages as well.


Python script takes input of four elements and outputs a valid 4 × 4 Sudoku grid in O(n) time, but only when given non-repeating elements Introduction

I've found a clever and fun thing to do after solving a n2 × n2 sudoku puzzle. I can take
a grid as such as this one and hard-code its indices as constraints to output other 4 × 4 latin squares in poly-time

Now, I took each successive row and hardcoded as indices with successive print statements.

Indices constraints

0123
3210
1032
2301

Working Code

print('enter with [1,2,3...] brackets')
text = input()[1:-1].split(',')
print(text[0], text[1], text[2], text[3])
print(text[3], text[2], text[1], text[0])
print(text[1], text[0], text[3], text[2])
print(text[2], text[3], text[0], text[1])

Question

Being a novice at python I'm asking is there a better way of hardcoding the Sudoku's pattern with fewer lines of code?

Because it would be daunting to have to write larger constraints for larger latin squares.

I would appreciate it to keep it O(n) time because I have a desire to input integers besides just elements 1-4. But, 100-104 and so on..

Use json to simplify your input handling.
You can use str.format to simplify all the prints.
You don't handle incorrect data well. What if I enter 3 or 5 numbers?
Your code doesn't run in \$O(n)\$ time, it runs in \$O(n^2)\$ time. I recommend that you ignore \$O\$ and just get working code if you're a novice. After you get it working make it readable. Finally here you should time your code to see if you need to then optimize it.
Use functions, they make your code easier to use.

import json

def handle_input(input_):
    try:
        data = json.loads(input_)
    except ValueError:
        raise ValueError('Invalid format.')

    if len(data) != 4:
        raise ValueError(f'Incorrect amount of numbers, got {len(data)} not 4.')
    return data

def main():
    print('Enter four numbers in brackets. E.g. [1, 2, 3, 4]')
    data = handle_input(input())
    print(
        '{0} {1} {2} {3}\n'
        '{3} {2} {1} {0}\n'
        '{1} {0} {3} {2}\n'
        '{2} {3} {0} {1}\n'
        .format(*data)
    )

if __name__ == '__main__':
    main()


Simple object comparer Creating a simple object compare function for public system types in a class meaning only looking at the first level of a class no breaking into an object and comparing sub properties.

It performs very quickly however I am unsure about the extensibility of it for the future should criteria evolve

public static IEnumerable<string> Compare<T1, T2>(this T1 Object1, T2 Object2) where T1 : IComparer
{
    if (Object2 == null)
        throw new NullReferenceException("Parameter object not instantiated");

    Type TypeOfObject1 = typeof(T1);
    PropertyInfo[] Object1Properties = TypeOfObject1.GetProperties(BindingFlags.Public | BindingFlags.Instance);

    Type TypeOfObject2 = typeof(T2);
    PropertyInfo[] Object2Properties = TypeOfObject2.GetProperties(BindingFlags.Public | BindingFlags.Instance);

    if (Object1Properties.Count() == 0 || Object2Properties.Count() == 0)
        throw new NullReferenceException("No public properties found");

    var DifferencesFound = Object1Properties
        .Join(Object2Properties,
                Object1Property => Object1Property.Name,
                Object2Property => Object2Property.Name,
                (Object1Property, Object2Property) => new { Object1Property = Object1Property, Object2Property = Object2Property })

        .Where(x => ((Type.GetTypeCode(x.Object1Property.PropertyType) != TypeCode.Object
                        && Type.GetTypeCode(x.Object2Property.PropertyType) != TypeCode.Object
                    )
                    && !x.Object1Property.GetValue(Object1).Equals(x.Object2Property.GetValue(Object2)))
                )
        .Select(x => x.Object1Property.GetCustomAttributes(typeof(DisplayNameAttribute), false).SingleOrDefault() as string ?? x.Object1Property.Name)
        .AsEnumerable();

    return DifferencesFound;
}

Note: Reads DisplayName attribute if it is not found then uses name

First of all the name, Compare doesn't explain what this method does (and I had to read all the code to have an idea). Don't be afraid of longer names like GetMatchedPropertiesWithDifferentValues() (or something like that...).

Maybe you have a reason for that, in code you're not showing, but there is no requirement for T1 to implement IComparer (I'd guess IComparable but...IComparer?)

Names reloaded. Usually local variables (and parameters) are camelCase.

Both Object1 and Object2 can be null, check for both. You should never ever throw NullReferenceException, if a function argument is null and it should not then you have to throw ArgumentNullException.

This code:

Type TypeOfObject1 = typeof(T1);
PropertyInfo[] Object1Properties = TypeOfObject1.GetProperties(BindingFlags.Public | BindingFlags.Instance);

Can be simplified to:

var object1Properties = typeof(T1).GetProperties();

However if caller specifies the generic types like this (just an example...):

Compare<Base, Base>(new Derived(), new Derived());

You may get the wrong result (what do you expect here? To get properties of Base or of Derived?) Just be aware of this (and document it).

You call Count() on PropertyInfo[]. LINQ implementation is smart enough but it really has no sense to avoid using Length property. Again do not throw NullReferenceException. Is there any null object? No, just an object without public properties then you can throw ArgumentException (possibly specialized for the two parameters).

You may want to filter properties to feed Join() (what you're doing later in Where() clause) before the Join(). Also, move it to a separate GetEligiblePropertiesForComparison() method.

Only when T1 and T2 coincide (maybe a separate overload?): if you just get the intersection of the two lists you can skip creating the temporary objects.

Is there a reason you do not compare reference types? Checking TypeCode works pretty well but moving this check to a separate method is even better. Also consider x.PropertyType.IsPrimitiveType || x.PropertyType == typeof(string) (if it's what you meant, comparison of DBNull can be tricky).

You might want to move comparison to a separate method where you check for IEquatable<T> or IComparable<T> or IComparable implementation (defaulting to EqualityComparer<T> if types coincide).

Name selection can be simplified:

x.Object1Property.GetCustomAttribute<DisplayNameAttribute>()?.DisplayName
    ?? x.Object1Property.Name;

Why don't you move it into a separate method string GetPropertyDisplayName(PropertyInfo)?

Here you do not need AsEnumerable().


Slow performance in looping and filling missing data I wrote some code that loops for specific data and then fills the missing cells in another sheet. The code works perfectly but it takes too much time to fill the missing cells (values).

What I tried to do is to Test if Cell B is blank or not , then I created 2 variables :

x for activesheet values which need to be filled and
y for sheet 1 (source) and keep comparing until x match y to take the value in front of the specific data.

The code I came up with:

  Sub TraiterNoms()
  Application.ScreenUpdating = False
  Application.DisplayAlerts = False
  Dim i As Variant
  Dim CompareRange As Variant
  Dim x As Variant
  Dim y As Variant
  Dim derlignE As Variant
  Dim derlignC As Variant

  derlignE = Range("A" & Rows.Count).End(xlUp).Row
  derlignC = Sheets("Feuil1").Range("B" & Rows.Count).End(xlUp).Row
  Set CompareRange = Sheets("Feuil1").Range("A:A").resize(derlignC, 1)

   For i = Range("A" & Rows.Count).End(xlUp).Row To 1 Step -1
    If Range("B" & i) = "" Then
      For Each x In Range("A:A").resize(derlignE, 1)
         For Each y In CompareRange
         If x = y Then x.Offset(0, 1) = y.Offset(0, 1)
        Next y
       Next x
      End If
    Next i
 Application.ScreenUpdating = True
 Application.DisplayAlerts = True
End Sub

I feel like I could improve this code to make it more fluent. Would that be over-complicated?

What's there to say about this code?

It looks like you just need VLookup. In column B1 of your active sheet you'll want =VLookup(A1,Feuil1!$A$1:$B$4,2,0). Make sure to edit Feuil1!$A$1:$B$4 to be the entire range you want. If your range is contiguous, no empty cells, Control+Shift+Down followed by Holding Shift+RightArrow ought to get you what you're after.

Edit:
Below you'll find code that does what you're attempting.

The reason why your initial code is so slow is you have a loop For Each ... Next within another loop. Inside this is checking each and every single one. and must go through the both lists. If in your first loop (ActiveSheet) you have 10 entries and your second loop (Feuil1) you have 15 entries you have to compare 150 times. If either list grows it will get slower and slower. By
 refactoring you have the same outcome but achieved by a better, faster, way.

Strive for code that is self documenting. What is occurring should be apparent when you read it. Minimal effort should be needed to understand what is going on.

The code below has a main Sub that sets some variables and calls on functions to provide information to complete the required task.

Public Sub TraiterNoms()
    Application.ScreenUpdating = False
    Application.DisplayAlerts = False

    Dim actSheet As Worksheet
    Set actSheet = ActiveSheet

    Dim sourceSheet As Worksheet
    Set sourceSheet = ThisWorkbook.Worksheets("Feuil1")

    Dim lastFormulaRow As Long
    lastFormulaRow = actSheet.Cells(Rows.Count, 1).End(xlUp).Row
    Dim formulaRange As Range

    On Error GoTo NoBlankCells
    Set formulaRange = actSheet.Range(actSheet.Cells(1, 2), actSheet.Cells(lastFormulaRow, 2)).SpecialCells(xlCellTypeBlanks)
    On Error GoTo 0

    Dim lookupFormula As String
    lookupFormula = GetLookupFormula(formulaRange, sourceSheet, 2)
    formulaRange.Formula = lookupFormula
    Dim subArea As Range
    For Each subArea In formulaRange.Areas
        subArea.Value2 = subArea.Value2
    Next

CleanExit:
    Application.ScreenUpdating = True
    Application.DisplayAlerts = True
    Exit Sub

NoBlankCells:
    Resume CleanExit

End Sub

Private Function GetLookupFormula(ByVal depositRange As Range, ByVal lookupSheet As Worksheet, ByVal lookupColumn As Long) As String
    Dim valueToMatch As String
    valueToMatch = depositRange.Cells(1, 1).Offset(ColumnOffset:=-1).Address(False, False)

    Dim lookupCells As String
    lookupCells = GetLookupCells(lookupSheet, lookupColumn)
    GetLookupFormula = "=Vlookup(" & valueToMatch & "," & lookupCells & "," & lookupColumn & ",0)"
End Function

Private Function GetLookupCells(ByVal sourceSheet As Worksheet, ByVal lookupColumn As Long) As String
    Dim lastRow As Long
    lastRow = sourceSheet.Cells(Rows.Count, "B").End(xlUp).Row

    Dim sourceRange As Range
    Set sourceRange = sourceSheet.Range(sourceSheet.Cells(1, 1), sourceSheet.Cells(lastRow, lookupColumn))

    GetLookupCells = sourceRange.Parent.Name & "!" & sourceRange.Address
End Function


Review of a C++ missing characters snippet I recently had to write a function to return characters missing from an input string.  Here is the snippet

#include <bitset>
#include <string>

template<class Iterator>
std::string getMissingLetters(Iterator begin, Iterator end) {

    std::bitset<26> letters;

    for (auto it = begin; it != end; ++it) {
        size_t c = tolower(*it) - char('a'); // normalized to start at 'a'
        if (c > 26) continue;
        letters.set(c);
        if (letters.to_ulong() == ((1 << 26) - 1)) break;  // all 26 bits set
    }

    std::string missing;
    for (int i = 0; i < letters.size(); ++i) {
        if (!letters[i]) missing.push_back(char('a' + i));
    }
    return missing;
}

I thought of couple things to improve, for example avoiding branch by clamping characters, eg.

letters.set(std::min(c, 26));

and perhaps blocking the loop s.t. the line

if (letters.to_ulong() == ((1 << 26) - 1)) break;

is executed every N iterations rather than each iteration.

What do you think one could have done better?

Instead of starting from a zero-ed bitset and then set them to true one by one, you could start with all true, set to false one by one and then use std::bitset::none() to break the loop.

But, apart from this small thing, std::string will probably preallocate memory (on the heap or on the stack; it's implementation dependent), whether or not you add a letter.  My advice would be to use an std::array with a fixed size of 26 (or your own stack implementation of a string) to directly store the letters, and remove the bitset and std::string.

That should simplified the code.  As the 26 chars will be stored in 7 int, copying or overwriting that size from the stack should be blazing fast (the cache lines of a modern processor is 32 bytes) compared to multiple math operations and a potential heap allocation.


Unmarshaling dynamic JSON I have JSON object response returned from the server which is similar to this one:

{
    "id": 123,
    "from_id": 234,
    "attachments": [
        {
            "type": "audio",
            "audio": {
                "artist": "Van Halen",
                "title": "Eruption"
            }
        },
        {
            "type": "photo",
            "photo": {
                "id": 123,
                "url": "http://someurl.com"
            }
        }
    ]
}

and I want to unmarshal it to the following structure:

type Response struct {
    Id          int `json:"id"`
    FromId      int `json:"from_id"`
    Attachments []Attachment
}

where Attachment is described like this

type Attachment struct {
    Type   string
    Fields map[string]interface{}
}

and Fields field is map that contains fields from "photo" or "audio" object of attachment in JSON.

Here is the code I've written to accomplish my task:

package main

import (
    "encoding/json"
    "fmt"
    "log"
    "reflect"
    "strings"
    "unicode"
)

type Response struct {
    Id          int `json:"id"`
    FromId      int `json:"from_id"`
    Attachments []Attachment
}

type Attachment struct {
    Type   string
    Fields map[string]interface{}
}

func (this *Attachment) UnmarshalJSON(b []byte) (err error) {
    type innerAttachment struct {
        Type  string
        Audio map[string]interface{}
        Photo map[string]interface{}
    }

    inAt := innerAttachment{}
    *this = Attachment{}

    if err = json.Unmarshal(b, &inAt); err == nil {
        this.Type = inAt.Type

        a := []rune(inAt.Type)
        a[0] = unicode.ToUpper(a[0])

        v := reflect.ValueOf(inAt)

        m := v.FieldByName(string(a)).Interface()
        this.Fields = m.(map[string]interface{})
    }

    return
}

func main() {
    encoded := `
    {
        "id": 123,
        "from_id": 234,
        "attachments": [
            {
                "type": "audio",
                "audio": {
                    "artist": "Van Halen",
                    "title": "Eruption"
                }
            },
            {
                "type": "photo",
                "photo": {
                    "id": 123,
                    "url": "http://someurl.com"
                }
            }
        ]
    }
    `

    response := &Response{}
    reader := strings.NewReader(encoded)
    err := json.NewDecoder(reader).Decode(&response)
    if err != nil {
        log.Fatal(err)
    }

    for _, v := range response.Attachments {
        fmt.Println(v.Type)
        for key, value := range v.Fields {
            fmt.Printf("%s => %s\n", key, value)
        }
        fmt.Println("")
    }
}

Is my code OK or maybe there's more Go'ish way to unmarshal JSON provided above?

I would not implement method UnmarshalJSON as it is not really necessary. Instead, I would create Audio and Photo types and put them as fields into Attachment type. It would be easier to reference data as field than get it as a value from map.

package main

import (
    "encoding/json"
    "fmt"
    "log"
    "strings"
)

type Response struct {
    Id          int `json:"id"`
    FromId      int `json:"from_id"`
    Attachments []Attachment
}

type Audio struct {
    Artist string `json:"artist"`
    Title  string `json:"title"`
}

type Photo struct {
    Id  int    `json:"id"`
    Url string `json:"url"`
}

type Attachment struct {
    Type  string
    Audio `json:"audio"`
    Photo `json:"photo"`
}

func main() {
    encoded := `
    {
        "id": 123,
        "from_id": 234,
        "attachments": [
            {
                "type": "audio",
                "audio": {
                    "artist": "Van Halen",
                    "title": "Eruption"
                }
            },
            {
                "type": "photo",
                "photo": {
                    "id": 123,
                    "url": "http://someurl.com"
                }
            }
        ]
    }
    `

    response := &Response{}
    reader := strings.NewReader(encoded)
    err := json.NewDecoder(reader).Decode(&response)
    if err != nil {
        log.Fatal(err)
    }

    fmt.Println(response)
}


Shortening System.out - which is better? Oftentimes people will shorten System.out to just out and make an analog called in (e.g. Scanner in = new Scanner(System.in);). I've seen two methods of doing this
Method 1:
import static java.lang.System.out;
Method 2:
static final PrintStream out = System.out;
Is there a coding convention or best practices for this? If so, which one is it and why is it more readable?

With the second approach you hide that out refers to System.out, so if anytime later you want to redirect the output, you need to change only static final PrintStream out = System.out; since the rest of the code is only aware of the fact that it is writing to a PrintStream.

The first approach is just using the shortcut offered by static imports.

Which one to use depends on the purpose of the code. If it is a one time, throwaway code trying out something, then I'd go with the first, otherwise the second, if there are many print statements. Otherwise I would not care replacing System.out.

On the other hand I would consider using a logger instead of System.out and that could be redirected to System.out to a File and to many other targets.


Remove duplicate elements from char array This is a method where a char[] will be supplied as an input and it should return an array, but without the duplicate elements. How can this be optimized?

private static char[] getMYCharArray(char[] array) {
        String myArray = "";
        for(int i = 0; i < array.length; i++) {
            if(myArray.indexOf(array[i]) == -1) // check if a char already exist, if not exist then return -1
                myArray = myArray+array[i];      // add new char
        }
        return myArray.toCharArray();
    }

One possibility is to use a Set. Just add the elements from the array and then get the unique elements from the Set. This algorithm will be O(n) instead of O(n^2) because each element is accessed only once.

Note that you can use Arrays.asList() to simplify the coding, too.


Async method with three internal tasks Consider we have three different database stores which we want to find an Id which is unique in all this three. I mean if we query first data store and we find that id. It's over and there is no need to further attempt, if that Id is in non of this three data stores we return null, I'm new in asynchronous programming, I wanted to know if there is any better implementation for this lines of code? I mean less if/else?

 public async Task<WebData> GetDataByIdAsync(GetDataById input)
    {
        var twitterData = await DataAccess.FindDataById<TwitterData>(input.Id);
        WebData result;
        if (twitterData  == null)
        {
            var facebookData = await DataAccess.FindDataById<FacebookData>(input.Id);
            if facebookData == null)
            {
                var linkedinData = await DataAccess.FindDataById<LinkedinData>(input.Id);
                if linkedinData == null)
                {
                    return null;
                }
                result = linkedinData
            }
            else
            {
                result = facebookData;
            }
        }
        else
        {
            result = twitterData ;
        }
        return result;
    }

Short and clean using null-coalescing operator:

public async Task<WebData> GetDataByIdAsync(GetDataById input)
{
    return await DataAccess.FindDataById<TwitterData>(input.Id)??
           await DataAccess.FindDataById<FacebookData>(input.Id)??
           await DataAccess.FindDataById<LinkedinData>(input.Id);
}


Strange performance difference For exercise, I wrote this code:

def BigPrime(numP, plist = [], x = 1, counter =0):
    while len(plist) <= numP:
        if x == 1 or x == 2 or x == 5:
                plist.append(x)
        elif x % 2 is not 0 and int(str(x)[-1]) is not 5:
                for div in plist[1:]:
                        if x % div == 0:
                                break
                else:
                        plist.append(x)
                counter += 1
        x += 1
    return plist, counter

It is a simple algorithm that should return a list of the first n primes.  When I tried to optimize it, I wanted to cut down all the possible already know not primes from the range.  That brought me to:

int(str(x)[-1]) is not 5

But then when I timed it, I saw that the code ran slower. Why?  It worked, and the for iterations were less indeed, but yet it was slower.

Also, when it comes to optimization, I'm a black hole of ignorance.  Any further way I could improve it?

Well for starters:

int(str(x)[-1]) is a really expensive operation:

Go from integer (x) to a string that represents X
Get the last element as a char [-1]
Transform that char back to an integer

Given what you're trying to do (find if the last digit is a 5), the same operation can be achieved with x%10==5 or x%51=5 (all integer operations, way less expensive).  This could be damaging your performance a lot.

Doing a quick experiment:

$> def test1():
    k = time.clock()
    s = int(str(55)[-1]) is not 4
    t = time.clock()
    print(t-k)

$> def test2():
     k = time.clock()
     s = (55%10 != 4)
     t = time.clock()
     print(t-k)

$> test1()
1.682255089008322e-05
$> test2()
1.7107678900174506e-06

test2() takes one order of magnitude less than test1() just because of the type of operations.

For your problem space, I think trying to find useful properties of prime numbers that you can exploit is the best way to optimize.  For example, a number N doesn't have a prime divisor greater than Sqrt(N).


Virtual Airline Dispatch Generator This Code is old and I am trying to figure out how to improve it. I am considering running the distance function in MySQL, but it is a ***** to figure out for me due to my experience with arithmetic in MySQL.

$id = $_GET['id'];
    $data = $_SESSION[$id];
    $airline = $data['airline'];
    $hub = $data['hub'];
    $ident = $data['originAirport'];
    $type = $data['type'];

//Query for Origin Airport Details
$query_org = $db->prepare("SELECT `ident`, `name`, `latitude_deg`, `longitude_deg`, `elevation_ft` FROM `airports` WHERE `ident` = :ident");
$query_org->bindParam(":ident", $ident);
$query_org->execute();
$originDetails = $query_org->fetch(PDO::FETCH_ASSOC);
$query_des = $db->prepare("SELECT `ident`, `type`, `name`, `latitude_deg`, `longitude_deg`, `elevation_ft` FROM `airports` WHERE `type` = :type");
$query_des->bindParam(":type", $type);
$query_des->execute();
$num = 0;
while ($destinDetails = $query_des->fetch(PDO::FETCH_ASSOC)) {
    $distance = distance($originDetails['latitude_deg'], $originDetails['longitude_deg'], $destinDetails['latitude_deg'], $destinDetails['longitude_deg'], "N");
    if ($distance >= $data['dismin'] && $distance <= $data['dismax']) {
        $a[$num] = array(
            'ident'         => $destinDetails['ident'],
            'type'          => $destinDetails['type'],
            'name'          => $destinDetails['name'],
            'elevation_ft'  => $destinDetails['elevation_ft'],
            'distance'      => $distance
        );
        $num++;
    }
}

$num = null;
$num = 0;

foreach ($a as $z) {
    $i = $z['ident'];
    $query_run = $db->prepare("SELECT `length_ft` FROM `runways` WHERE `airport_ident` = :ident");
    $query_run->bindParam(":ident", $i);
    $query_run->execute();
    $result = $query_run->fetch(PDO::FETCH_ASSOC);
    $length = $result['length_ft'];
    if ($length >= $data['minrunlen'] && $length <= $data['maxrunlen']) {
        $b[$num] = array(
            'icao' => $z['ident'],
            'name' => $z['name'],
            'distance' => $z['distance']
            );
        $num++;
    }

}

$count = count($b);

$int = rand(0, $count);

//Query for Flight_ID
$queryc = $db->prepare("SELECT `fid` FROM `dispatches` ORDER BY `id` DESC");
$queryc->execute();
$flight_id = $queryc->fetch(PDO::FETCH_NUM);
if ($flight_id[0] !== 1000) {
    $flight_id = (int)$flight_id[0];
    $flight_id++;
} else {
    $flight_id = 1000;
}
$dispatch = array(
    'airline'       => strtoupper($data['airline']),
    'hub'           => strtoupper($data['hub']),
    'pid'           => $data['pid'],
    'pilot'         => $data['fname'].' '.$data['lname'],
    'email'         => $data['email'],
    'fid'           => $flight_id,
    'airframe'      => $data['airframe'],
    'datetime'      => date("Y-m-d H:i:s"),
    'connumb'       => md5(rand(0,9999999999)),
    'originICAO'    => $originDetails['ident'],
    'originName'    => utf8_encode($originDetails['name']),
    'destinICAO'    => $b[$int]['icao'],
    'destinName'    => utf8_encode($b[$int]['name']),
    'distance'      => $b[$int]['distance']."nm"
);
//Query for runway information
$destinICAO = $dispatch['destinICAO'];
$queryd = $db->prepare("SELECT `length_ft`, `width_ft`, `lighted`, `closed`, `le_ident`, `le_latitude_deg`, `le_longitude_deg`, `le_elevation_ft`, `le_heading_degT`, `le_displaced_threshold_ft`, `he_ident`, `he_latitude_deg`, `he_longitude_deg`, `he_elevation_ft`, `he_heading_degT`, `he_displaced_threshold_ft` FROM `runways` WHERE `airport_ident` = :ident");
$queryd->bindParam(":ident", $destinICAO);
$queryd->execute();

Any suggestions are helpful. I am not looking for anybody to rewrite the code. That is my job, but guidance is more what I am looking for.

Prepare once - execute multiple times

Prepared statements not only prevent SQL injection, but also enable you to only prepare a statement once, and then execute it multiple times with different values for increased performance:

$query_run = $db->prepare("SELECT `length_ft` FROM `runways` WHERE `airport_ident` = :ident");
$query_run->bindParam(":ident", $i);

foreach ($a as $z) {
    $i = $z['ident'];
    $query_run->execute();
    [...]
}

Naming

don't use short, non-expressive variable names. a, z, i (as non-loop variable), b, etc are not telling a reader much.
use either camelCase or snake_case, but don't mix them.
write out words. what's an org or a des? what is a destin? why is ident an originAirport?
What's a num? (ok, a number, but what kind of number? Seems to be a counter, so counter would be more fitting. But what does it count?
int is also not a very good name. It seems to be a random index? Then randomIndex would be better.

Misc

variables which are only used in one place, and whos name doesn't really add anything can be removed. Eg $count = count($b); is not needed, just use this directly: $int = rand(0, count($b));
if you split your code up into methods, it will be easier to read. You might have methods such as getOriginAirport, meassureDistancesFrom, getFlightId, getRunwayInfo, etc.


Print binary search tree by levels function implementation I do not like this implementation as is seems too much complicated.  My idea was to implement some BFS algorithm variant.

Some points to notice:

I use one queue.
In the begining put the tree root in the queue and dummy treeNode delimiter which is recognized by its data == -1.
(I need this for distiguishing between tree levels).
After each level I assign -1 to the queue.

Any suggestions of how to simplify it?  (Currently seem to work good)

public void printTreeByLevels() // Should be BFS variant implementation
{
    int levelCounter = 0;
    boolean currNodeHasChild = false;
    Queue<TreeNode> q = new ArrayDeque<TreeNode>();

    q.add(m_Root);
    q.add(new TreeNode(-1));
    System.out.print("Level:" + levelCounter+ " ");

    while (!q.isEmpty())
    {
        TreeNode currentNode = q.poll();
        currNodeHasChild = false;
        if(currentNode.m_Data == -1)
        {
            TreeNode nextQueueNode = q.peek();
            while(nextQueueNode!= null && currentNode.m_Data == -1)
            {
                currentNode = q.poll(); //bypassing delimiters
            }

            if(currentNode.m_Data != -1)
            {
                levelCounter++;
                System.out.println();
                System.out.print("Level:" + levelCounter+" ");
            }
            else if(nextQueueNode == null)
            {
                break;
            }

        }

        System.out.print("\t"+currentNode.m_Data);

        if(currentNode.m_LeftNode!= null)
        {
            q.add(currentNode.m_LeftNode);
            currNodeHasChild = true;
        }

        if(currentNode.m_RightNode!=null)
        {
            q.add(currentNode.m_RightNode);
            currNodeHasChild = true;
        }

        if(currNodeHasChild)
        {
            q.add(new TreeNode(-1)); // sign of next level
        }
    }
}

General Notes

Let me preface this section with the fact that I have not looked at Java conventions in a while. Some of these suggestions may stray from classic Java conventions.

With that off my chest, visually your code looks pretty good, though mixture of underscores in your hungarian notation is a little bit odd (Thanks Vogel612).

// Could you not just have 'currentNode.mRightNode'?
if(currentNode.m_RightNode!=null)

You also have several loops or if-statements with only a single line of code in them. This may just be preference, but if you remove the braces in those cases:

if(currNodeHasChild)
{
    q.add(new TreeNode(-1)); // sign of next level
}

becomes

if(currNodeHasChild)
    q.add(new TreeNode(-1)); // sign of next level

which makes the code more compact without reducing readability.

NOTE: When using no-brace notation, only ONE LINE after the if-statement, for-loop, etc. will be considered 'inside' that block of code. As Vogel612 pointed out in a comment below, there have been cases where this notation was misused. Keep an eye out for these bugs, as they can pop up relatively easily.

One final thing: I do not prefer in-line comments. To my eyes, they break up the flow of the program visually.

BFS Solution

Now onto code improvements.

This problem is built to be solved by recursion. Combining recursion and a breadth-first search yields a relatively simple solution:

public void printTreeByLevels(Queue<TreeNode> nodes, int level)
{
    System.out.println("Level " + level);

    // New queue to hold child nodes
    Queue<TreeNode> newNodes = new ArrayDeque<TreeNode>();

    // Loop through all nodes in the queue
    for(TreeNode node : nodes)
    {
        System.out.println("\t" + node.m_Data);

        // Add child nodes if they exist
        if(node.m_LeftNode != null)
            newNodes.add(node.m_LeftNode);
        if(node.m_RightNode != null)
            newNodes.add(node.m_RightNode);
    }

    // Call this function again only if one or many child node(s) were found
    if(!newNodes.isEmpty())
        printTreeByLevels(newNodes, level+1);
}

A single for-loop and some if-statements solve this problem quite simply and easily.

NOTE: I originally wrote this in Python as its been a while since I wrote Java. However, the syntax should all be correct.


Class design - Excel report I am working on a C#.Net application that will generate an Excel report from a template.  Although the code's not in the app yet, it will pull data from a SQL Server database to populate the report.

I am asking for help in reviewing my class design.  My code is below, with some details of the functions removed for simplicity (if someone feels like they need to see them in order to help, I can add them).

using System;

namespace RGBBProgramSummary
{

class Program
{

    static void Main()
    {
        ReportSheet ProgramSummary = new ReportSheet("Program Summary");

        ProgramSummary.CleanUp();
    }

}
}

using System;
using System.IO;
using System.Runtime.InteropServices;
using Microsoft.Office.Interop.Excel;

namespace RGBBProgramSummary
{

class ReportBook
{
    public bool Error;
    public DateTime RunDate;
    private string FileName;

    static Application ExcelApp = new Application();
    public Workbooks reportWBs = ExcelApp.Workbooks;
    public Workbook ReportWB;

    public ReportBook()
    {
        RunDate = GetRunDate();
        FileName = GetFileName(RunDate);

        File.Copy(Properties.Settings.Default.Template, FileName);

        ReportWB = reportWBs.Open(FileName);
        //ExcelApp.Visible = true;
    }

    private DateTime GetRunDate()
    {
        ...
        return DateTime.Now.AddDays(-(int)DateTime.Now.DayOfWeek - 2);
    }

    private String GetFileName(DateTime RunDate)
    {
        string FileName;

        ...
        return Properties.Settings.Default.ReportPath + FileName;
    }

}
}

using System;
using System.Globalization;
using System.Collections.Generic;
using System.Runtime.InteropServices;
using Microsoft.Office.Interop.Excel;

namespace RGBBProgramSummary
{
class ReportSheet : ReportBook
{
    private Worksheet reportWS;

    public ReportSheet(string SheetName)
    {
        reportWS = ReportWB.Worksheets[SheetName];

        BuildHeader(reportWS);
    }

    public void CleanUp()
    {
        ReportWB.Save();
        ReportWB.Close();

        Marshal.ReleaseComObject(reportWS);
        Marshal.ReleaseComObject(reportWBs);
        Marshal.ReleaseComObject(ReportWB);
    }

    private void BuildHeader(Worksheet WS)
    {

        int WeekColumn = 0;
        int MonthColumn = 0;

        ...
        int j = CountOfWeeks();
        ...
        List<string> MonthList = GetMonthList();
        ...
    }

    private int CountOfWeeks()
    {
        int RunDateWeek
        int FirstDateWeek

        ...
        return RunDateWeek - FirstDateWeek + 1;
    }

    private List<string> GetMonthList()
    {

        List<string> MonthList = new List<string>();

        ...
        return MonthList;
    }

}
}

As you can see, I really only have 2 classes.  ReportBook represents the Workbook itself, and ReportSheet represents the Worksheet (ReportSheet inherits from ReportBook).  I'm wondering if that's overkill, and really if they should be consolidated into one class.  At the least, I would need to make changes if I ever wanted to add an additional worksheet (for one, I wouldn't want to open the workbook in the constructor of the base class then).

Also, I was going create a function in the ReportSheet class to read from the aforementioned SQL DB.  But, after reading answers to another question, I'm thinking that the read from the DB should be in it's own class.

Although arguably a Pandora's box of other complications, have you considered ditching the Interop libraries and driving this through Ace/Jet drivers via OleDB?  Too often do I see well-intentioned applications fall over because Excel gets upgraded.  Best-case scenario you'll have to manage multiple releases, whereas oftentimes you're stuck redeveloping because of core changes that break existing methods.

Even when I need specific functions available only to the Interop libraries, I try to avoid using an actual reference in my project.  You can dynamically marshal your Excel.Application object if you're building this in .NET 4.0, and it gives you some flexibility depending on what you're trying to achieve.

If you're just interested about data transfer, I'd consider making a move to OleDB if you're comfortable with SQL-based database querying.

I think by omitting your specific methods of how the data gets into the sheet, you're leaving out one of the most important parts of the application in terms of performance.  Are you dumping the data cell by cell?  I'm assuming coming from SQL the data will be in DataSet/DataTable format, so you could dump to an array, and then point to an address in the sheet.

I do like that you've avoided the dreaded double-dot, and are taking the proper Marshalling release methods to make sure Excel goes away when it's supposed to.

/$.02

-- Locke


Find a palindromic string in C This program tests whether a string is a palindrome or not.
Can there be any improvements or shortcuts that can be used in the program. You can give any type of string as input. The program finds does it work.

#include <stdio.h>
#include <string.h>

/*
A palindrome is a string that is same in both forward and backward reading.
Example:
   "madam"
   "racecar"
   "a man a plan a canal panama"
   "radar"
You will write a program that will test if a given string is a palingdrome or not.
Your program will ask the user to input a string and if the string is a palindrome program
will just print "Yes, it is a Palindrome", otherwise will print "No, not a Palindrome".

Please note that:
1. Your you need to check in case-insensitive way, that means: Madam or madam both should be
detected as Palindrome.

2. There can be (any number of ) spaces in between the words.
    "A man a plan a canal panama"
        OR
    "A     man    a   pla n a cana l Panama"
    both the strings must be detected as Palindrome.
3.There can be punctuations in between the words, for this assignments,
we consider only 4 punctuations,   . ?  ! and ,

Your program will just need to ignore them (treat them as space).
    "Cigar? Toss it in a can. It is so tragic."
    Should be detected as palindrome.

 *** For this assignment I will not write any instructions or guidance, you are free
        to implement it with your own way, you can use the string.h functions

    Good luck.

*/

/***********************************************************************

    Created by Shaik Mohammed Zeeshan
    Date - 19 Aug 2018
    Objective - Checks whether a given string is palindrome or not

************************************************************************/

int main()
{
    char string[100];
    char string1[100];//the string without spaces and special characters will be stored in this

    printf("Enter a string: ");
    scanf("%[^\n]", string);

    int isPalindrome = 1;  // assign 0 to this if the string is a NOT palindrome

    // write code to test if string is a palindrome

    int index;
    int index1;//index for the second array

    for(index=0,index1=0;string[index] != '\0'; index++)  //to eliminate spaces, special characters and capital letters from the string
    {
        if( (string[index] < 'A' || string[index] > 'Z' ) && (string[index] < 'a' || string[index] > 'z' ) ) // checks if the element is a special
            continue;                                                                              // character or space and skips the iteration

        if(string[index] >= 'A' && string[index] <= 'Z' )
            string[index] += 32;

        string1[index1] = string[index];
        index1++;
    }
    string1[index1] = '\0'; // assigning the last element with null character

    int i,stringlength = strlen(string1); // storing length after eliminating unecessary characters

    for(i=0;i<=stringlength/2;i++) //starts the loop from first element to the middle element
    {
        if(string1[i] != string1[stringlength-i-1]) //checks if the elements are true
        {
            isPalindrome = 0;   // checks the first  element with the first element from the last and second element with the penultimate element and so on
            break;
        }
    }

    // at the end you need to test
    if (isPalindrome)
        printf("Yes, it is Palindrome!\n");
    else
        printf("No, not a Palindrome\n");

    return 0;
}

The amount of comments you use is more distracting than helpful. Good comments are those that explain what the overall logic of a block of code is, they don’t explain each line of code. Code is supposed to be self-documenting, that is, the code should explain itself. You accomplish this by using good variable names and good function names, which explain their own meaning.

Also, you need to use the functionality in the standard library. For example, to test whether a character is a normal letter or not, use isalpha rather than the four comparisons you’re using (against 'a', 'z', etc.)


Creating an area graph in iOS I am trying to create an area graph from the upper and lower limit values.

First I created two line graph from those points as:

Sample data points are:

 var line1dataPoints:[CGPoint] = [CGPoint(x: 10, y: 120),CGPoint(x: 20, y: 130),CGPoint(x: 20, y: 120),CGPoint(x: 70, y: 130),CGPoint(x: 70, y: 120),CGPoint(x: 80, y: 130),CGPoint(x: 90, y: 120),CGPoint(x: 100, y: 110),CGPoint(x: 120, y: 120),CGPoint(x: 120, y: 130)]
 var line2dataPoints:[CGPoint] =  [CGPoint(x: 10, y: 170),CGPoint(x: 20, y: 180),CGPoint(x: 20, y: 170),CGPoint(x: 70, y: 180),CGPoint(x: 70, y: 170),CGPoint(x: 80, y: 180),CGPoint(x: 90, y: 170),CGPoint(x: 100, y: 180),CGPoint(x: 120, y: 160),CGPoint(x: 120, y: 170)]

Method to create those line graphs are:

func addFirstLine(){

        let graphPath = UIBezierPath()
        graphPath.moveToPoint(line1dataPoints[0])

        for i in 1..<line1dataPoints.count {

            let nextPoint = line1dataPoints[i]
            graphPath.addLineToPoint(nextPoint)

        }
        let lineLayer = CAShapeLayer()
        lineLayer.lineWidth = 1.0
        lineLayer.path = graphPath.CGPath
        lineLayer.strokeColor = UIColor.blueColor().CGColor
        lineLayer.fillColor = UIColor.clearColor().CGColor
        self.view.layer.addSublayer(lineLayer)

    }

    func addSecondLine(){

        let graphPath = UIBezierPath()
        graphPath.moveToPoint(line2dataPoints[0])

        for i in 1..<line2dataPoints.count {

            let nextPoint = line2dataPoints[i]
            graphPath.addLineToPoint(nextPoint)

        }
        let lineLayer = CAShapeLayer()
        lineLayer.lineWidth = 1.0
        lineLayer.path = graphPath.CGPath
        lineLayer.strokeColor = UIColor.redColor().CGColor
        lineLayer.fillColor = UIColor.clearColor().CGColor
        self.view.layer.addSublayer(lineLayer)
    }

I created an area graph with the method as:

func createAreaGraph(){

    let rev = Array(line2dataPoints.reverse())
    line1dataPoints.appendContentsOf(rev)

    let graphPath = UIBezierPath()
    graphPath.moveToPoint(line1dataPoints[0])

    for i in 1..<line1dataPoints.count {

        let nextPoint = line1dataPoints[i]
        graphPath.addLineToPoint(nextPoint)

    }
    let lineLayer = CAShapeLayer()
    lineLayer.path = graphPath.CGPath
    lineLayer.fillColor = UIColor.grayColor().CGColor
    self.view.layer.addSublayer(lineLayer)
}

The results:

However, I am wondering if this is the right way to create an area graph or not.

To me, this seems like a perfectly good way to create the area graph! I think you've done something that's simple, straightforward, and easy to understand. Nice work!

Don't Repeat Yourself

The one thing that I think could be improved is the drawing of the line graphs. You have 2 functions which look identical except one is using line1dataPoints and the other is using line2dataPoints as its input. If you ever decide that something needs to change in the drawing or you need to add or remove anything, you now have to do it in 2 places. Why not just have a single function and pass in the array you want to use? Something like this:

func drawLineGraph(let dataPoints: [CGPoint]) {

    let graphPath = UIBezierPath()
    graphPath.moveToPoint(dataPoints[0])

    for i in 1..<dataPoints.count {

        let nextPoint = dataPoints[i]
        graphPath.addLineToPoint(nextPoint)

    }

    let lineLayer = CAShapeLayer()
    lineLayer.lineWidth = 1.0
    lineLayer.path = graphPath.CGPath
    lineLayer.strokeColor = UIColor.redColor().CGColor
    lineLayer.fillColor = UIColor.clearColor().CGColor
    self.view.layer.addSublayer(lineLayer)
}

Naming

You probably noticed that I renamed the function above to something more descriptive. Your original name was kind of vague. addFirstLine() leaves me wondering what you're adding it to?

But you are actually adding a layer to something at the end of the function. That's kind of surprising! Your original name implied a line was being added to... something. But really what's being added is a layer! And this is a side effect of the function!

Avoid Side Effects

If you want to add a layer to another layer, you should make that explicit in the function's name and definition. You should be either returning the layer that needs to be added, or passing in the layer to add the new layer to. As it is, this function creates a drawing path, then creates a layer to draw it into, then adds it to another layer. But the function definition looks like it just examines some data points. A better function definition might look something like this:

func addLineLayer(let dataPoints: [CGPoint], to layer: CALayer)

Now when you call it, you'll write:

addLineLayer(line1DataPoints, to:self.view.layer)

and it's much more clear to a reader what's going on. (Personally, I'd rename line1DataPoints to something more descriptive, too. Data points from what? Do they represent some maximum or minimum values?


Click! program in Java Swing I've created a program where the user is, at first, presented with a window with a yellow circle (random size, random coordinates) and a Start button. Once the user presses Start, the circle, when hovered over, will turn red in color and when it is clicked, another circle (random size, random coordinates) will be generated and this process is repeated (without the Start button).

The circle is generated with a random size of minimum 10 and maximum 100. The circle will be fully visible in the JFrame.

Is there any room for improvement?

import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.JButton;
import javax.swing.UIManager;
import javax.swing.UnsupportedLookAndFeelException;

import java.awt.EventQueue;
import java.awt.Graphics;
import java.awt.Dimension;
import java.awt.Color;

import java.awt.geom.Ellipse2D;

import java.awt.event.MouseEvent;
import java.awt.event.MouseAdapter;
import java.awt.event.ActionListener;
import java.awt.event.ActionEvent;

import java.util.Random;

public class ClickGame {
    public static void main(String[] args) {
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                try {
                    UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());
                } catch (ClassNotFoundException | InstantiationException | IllegalAccessException | UnsupportedLookAndFeelException ex) {
                    ex.printStackTrace();
                    System.exit(-1);
                }

                JFrame frame = new JFrame("Game - Click the circle!");
                frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                frame.add(new Panel());
                frame.pack();
                frame.setLocationRelativeTo(null);
                frame.setResizable(false);
                frame.setVisible(true);
            }
        });
    }
}

class Panel extends JPanel {   /* Main panel of Click game */

    Ellipse2D.Double circle;

    boolean hover = false;

    int randomSize = 0;
    int randomPositionX = 0;
    int randomPositionY = 0;

    public final static int MAX_SIZE = 100;
    public final static int MIN_SIZE = 10;

    public final static int WIDTH    = 500;
    public final static int HEIGHT   = 500;

    private Random rand = new Random();

    public Panel() {
        init();
        generateRandom();
    }

    public void init() {
        final JButton start = new JButton("Start");
        start.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e)
            {
                addMouseMotionListener(new MouseAdapter(){
                    public void mouseMoved(MouseEvent e) {
                        if(circle.contains(e.getPoint()))
                            hover = true;
                        else
                            hover = false;
                        repaint();
                    }
                });

                addMouseListener(new MouseAdapter(){
                    public void mouseClicked(MouseEvent e) {
                        if(circle.contains(e.getPoint()))
                            generateRandom();
                    }
                });
                remove(start);
                repaint();
            }
        });
        add(start);
    }

    public void generateRandom() {
        hover = false;
        randomSize = MIN_SIZE + (rand.nextInt(MAX_SIZE - MIN_SIZE) + 1);

        randomPositionX = rand.nextInt(WIDTH - randomSize);
        randomPositionY = rand.nextInt(HEIGHT - randomSize);
        circle = new Ellipse2D.Double(randomPositionY, randomPositionX, randomSize, randomSize);

        repaint();
    }

    @Override
    public Dimension getPreferredSize() {
        return new Dimension(WIDTH, HEIGHT);
    }

    @Override
    public void paintComponent(Graphics g) {
        super.paintComponent(g);
        if(hover)
            g.setColor(Color.RED);
        else
            g.setColor(Color.YELLOW);
        g.fillOval(randomPositionY, randomPositionX, randomSize, randomSize);
    }

}

Using an IDE, some changes are suggested automatically. For example:
Runnable and ActionListener can be replaced with Lambdas,

        if(circle.contains(e.getPoint()))
            hover = true;
        else
            hover = false;

can be simplified to

hover = circle.contains(e.getPoint());

Also, is it really intended that in Ellipse2D.Double(randomPositionY, randomPositionX, randomSize, randomSize) the first parameter is Y?

Usually if/else statements are used with braces.

In such small classes, the distinction between UI logic and business logic may not seem important, but as programs grow, it also gets more important. The Methods in your MouseListener seem to contain logic which could be extracted to a class or method. Although I am not a Swing expert, IIRC the repaint() method has to be called from the Event-dispatching thread, and could be therefore moved from generateRandom() into the Listener-methods where they are guaranteed to be called from the Event-dispatching thread.

The logic for generating a new circle could be refactored into an own class like this:

class CircleGenerator{
    public final static int MAX_SIZE = 100;
    public final static int MIN_SIZE = 10;
    private Random rand = new Random();

    public Ellipse2D.Double generateRandomCircle() {

        int randomSize = MIN_SIZE + (rand.nextInt(MAX_SIZE - MIN_SIZE) + 1);

        int randomPositionX = rand.nextInt(WIDTH - randomSize);
        int randomPositionY = rand.nextInt(HEIGHT - randomSize);
        return new Ellipse2D.Double(randomPositionX, randomPositionY, randomSize, randomSize);
    }
}


Count of Smaller Numbers After Self This is a LeetCode.com problem.
https://leetcode.com/problems/count-of-smaller-numbers-after-self/description/

  You are given an integer array nums and you have to return a new
  counts array. The counts array has the property where counts[i] is the
  number of smaller elements to the right of nums[i].

  Example:

  Given nums = [5, 2, 6, 1]

  To the right of 5 there are 2 smaller elements (2 and 1). To the right
  of 2 there is only 1 smaller element (1). To the right of 6 there is 1
  smaller element (1). To the right of 1 there is 0 smaller element.

  Return the array [2, 1, 1, 0].

Following is my solution which is accepted. But I noticed my run time is lot higher than many other submissions. I am curious to know if there is a better algorithm to this problem or is it something in the implementation that I can improve? How pythonic is my code? Do you see issues with any logic that could be done in a better way (both performance and code beauty wise)

Algorithm:
I start traversing the input list from the end. I take each element and insert it into a BST. While inserting I also track for each node in the BST, how many nodes are left of it (the size of left subtree) which denotes elements that are strictly smaller than current node. I also compute a position variable that starts at zero at root and gets incremented by X, each time a node is visited with value less than the given node where X is the count of its left subtree size plus one. If the number is equal, I only increment it by the node's left subtree count.

class bst_node(object):

    def __init__(self, val):
        self.val = val
        self.left = None
        self.right = None
        self.left_count = 0 # less than

# Name of this class is given by LeetCode, not my choice.
class Solution(object):

    def add_node(self, bst_pointer, new_node, position):
        if bst_pointer == None:
            return position, new_node
        if new_node.val < bst_pointer.val:
            bst_pointer.left_count += 1
            position, bst_pointer.left = self.add_node(bst_pointer.left, new_node, position)
        else:
            if new_node.val > bst_pointer.val:
                position += 1
            position += bst_pointer.left_count
            position, bst_pointer.right = self.add_node(bst_pointer.right, new_node, position)
        return position, bst_pointer

    # This method signature is also given by Leetcode.
    def countSmaller(self, nums):
        """
        :type nums: List[int]
        :rtype: List[int]
        """
        res = []
        bst = None

        for n in nums[::-1]:
            smaller_after, bst = self.add_node(bst, bst_node(n), 0)
            res.append(smaller_after)

        return res[::-1]

# Few test cases ...
print(Solution().countSmaller([]))
print(Solution().countSmaller([5, 2, 6, 1]))

Your BST can become unbalanced. In the worst case when the input is sorted, the tree degenerates to a linked list, and you traverse through it all to insert each element. Various algorithms exist for self-balancing search trees, but you might also want to look into the SortedContainers library. Even if you want to create your solution from scratch, you can find good ideas there.

Using sortedcontainers.SortedList the solution becomes very simple:

def countSmaller(self, nums):
    result = []
    seen = sortedcontainers.SortedList()
    for num in reversed(nums):
        result.append(seen.bisect_left(num))
        seen.add(num)
    result.reverse()
    return result

This is twice as fast as yours for 300 random numbers, and the gap increases as the size grows. For list(range(100)) this is ten times as fast.