Command line utility to convert image formats of photos Below is some code for a basic command line utility to convert image formats of photos. I'm wondering how any part of it might be written with better style, i.e. how it could be neater, clearer and generally "better".

import os
import argparse
from PIL import Image

def get_arguments():
    ''' Get command line arguments '''
    parser = argparse.ArgumentParser(
        description='A command line tool to convert images to different formats')
    parser.add_argument(
        'input',
        help='full path to the input file or folder')
    parser.add_argument(
        'outext',
        choices=Image.registered_extensions().keys(),
        help='output extension for file')
    parser.add_argument(
        '-n', '--new-name',
        help='give the output file a new name')
    parser.add_argument(
        '-d', '--destination',
        help='set new destination directory')
    parser.add_argument(
        '-R', '--remove-input',
        action='store_true',
        help='also remove the input file')
    args = parser.parse_args()
    if not (os.path.isfile(args.input) or os.path.isdir(args.input)):
        # easier to check file validiy here and keep infile as str,
        # rather than use type checking in parser.add_argument()
        raise parser.error('Unrecognised input type - input must be a file or folder')
    else:
        return args

def convert_image(infile, outext, outname=None, outdir=None, remove_infile=False):
    ''' Convert an image to a new format and optionally:
          * Change the filename
          * Put the file in a new directory
          * Remove the input file
    '''
    indir, inname = os.path.split(infile)
    inname = os.path.splitext(inname)[0]
    if outname is None:
        outname = inname
    if outdir is None:
        outdir = indir
    outfile = os.path.join(outdir, '{}{}'.format(outname, outext))
    if not os.path.isdir(outdir) and outdir != '':
        os.makedirs(outdir)
    Image.open(infile).save(outfile)
    if remove_infile:
        os.remove(infile)

def convert_images(im_dir, outext, outname=None, outdir=None, remove_infile=False):
    ''' run convert_image() for all files in a folder '''
    for item in os.listdir(im_dir):
        if os.path.splitext(item)[1] in Image.registered_extensions().keys():
            convert_image(
                os.path.join(im_dir, item),
                outext,
                outname=outname,
                outdir=outdir,
                remove_infile=remove_infile)

def main():
    args = get_arguments()
    if os.path.isdir(args.input):
        convert_images(
            args.input,
            args.outext,
            outname=args.new_name,
            outdir=args.destination,
            remove_infile=args.remove_input)
    elif os.path.splitext(args.input)[1] in Image.registered_extensions().keys():
        convert_image(
            args.input,
            args.outext,
            outname=args.new_name,
            outdir=args.destination,
            remove_infile=args.remove_input)
    else:
        raise ValueError('Unsupported file format')

if __name__ == '__main__':
    main()

Specific suggestions:

I generally find that ordering functions by their importance is helpful. Moving get_arguments just above or below main would allow readers to get into the functionality immediately. Others prefer to write methods in call order, but this feels a bit arbitrary (should main be first or last?) and not always achievable, because with branching it's possible to have a cycle of callers.
*nix tools usually support any number of paths as their last arguments, operating on each of them. This would certainly be useful here, and means you could even avoid the whole if/else in main because all inputs should be file paths.
argparse has a file type which you can use.
Don't rely on file extensions to correspond to file formats. I would instead rely on PIL to detect the file type and to throw an exception if it doesn't know it.
Abbreviations like inname make the code harder to read. One way to avoid the urge to shorten everything is to use an IDE, because it'll help you auto-complete everything which is in scope.
In the end this script is a small convenience wrapper around PIL. This is one of those rare occasions when I would suggest replacing it with a small shell script.

General suggestions:

black can automatically format your code to be more idiomatic.
isort can group and sort your imports automatically.
flake8 with a strict complexity limit will give you more hints to write idiomatic Python:

[flake8]
max-complexity = 4
ignore = W503,E203

I would then recommend adding type hints everywhere and validating them using a strict mypy configuration:

[mypy]
check_untyped_defs = true
disallow_untyped_defs = true
ignore_missing_imports = true
no_implicit_optional = true
warn_redundant_casts = true
warn_return_any = true
warn_unused_ignores = true


Trying to use "this" to refer to a class's instance variable inside a function within the class's method in JavaScript Hope that title makes sense.
This is part of an object I have:
The event listener attaches to the correct marker passed in the parameter but inside the function, this points to something else.

function UserGoogleMap($mapElement) {

    this.$mapElement = $mapElement;
    this.map = {};
    this.marker = null;
    this.coords = new google.maps.LatLng(25.2697, 55.3095)

    this.initMap = function () {
       google.maps.event.addListener(this.marker, 'dragend', function() {
         console.log(this.marker); // Undefined
         pos = this.marker.getPosition();
         $('#google-map-info').html("Latitude:" + pos.lat() + " Longitude: " +  pos.lng());
       });
    }

This works with a local variable:

    var marker = this.marker;
    google.maps.event.addListener(marker, 'dragend', function() {
        pos = marker.getPosition();
        $('#google-map-info').html("Latitude:" + pos.lat() + " Longitude: " +  pos.lng());
    });

this.initMap();

}

Is there anyway to rewrite this to use this.marker? or am I stuck with creating a local variable to refer to it? Also is there a way to have this.initMap() execute automatically instead of having to add this.initMap(); at the end of the class?

Any other feedback would be appreciated.

The "that" solution works well, but you can also bind your function to this :

function UserGoogleMap($mapElement) {
  this.$mapElement = $mapElement;
  this.map = {};
  this.marker = null;
  this.coords = new google.maps.LatLng(25.2697, 55.3095)

  this.initMap = function () {
    google.maps.event.addListener(this.marker, 'dragend', function() {
      console.log(this.marker);
      pos = this.marker.getPosition();
      $('#google-map-info').html("Latitude:" + pos.lat() + " Longitude: " +  pos.lng());
    }.bind(this));
  }
}

There is a way to execute initMap directly :

this.initMap = function () {
  google.maps.event.addListener(this.marker, 'dragend', function() {
    pos = this.marker.getPosition();
      $('#google-map-info').html("Latitude:" + pos.lat() + " Longitude: " +  pos.lng());
    }.bind(this));
    return arguments.callee;
 }();

But arguments.callee is deprecated (due to performance issues), so calling initMap manually is better.


Improved Sound Control System After a bit of work, I've changed my code a lot from the previous question:

I was recommended by George Mauer and schism to make a better structural design, so hopefully this new build is a bit sleeker in design. I will note that my main target is optimal performance. But like Mr. Mauer said, it is most likely negligible in this particular case.

What I've attempted since is removing duplicate code at the same time optimizing performance and fixing design. Here is what I came up with:

var MediaMaster = new (function(){
  var sounds = {};// a container of Audio instances and names(keys) as a reference to their respective values.

  // Master Controll of sounds
  var liveSounds = [],
      instanceVolume = {},
      globalVolume = 1;

  var SB = {muted: false, previousVolume:1};
  this.soundBatch = this.sBatch = SB;
  SB.toggleMute = function(){
    if( SB.muted === false ){
      // mute everything
      liveSounds.forEach(function(snd){
        snd.previousVolume = snd.volume;
        snd.volume = 0;
      });
      SB.previousVolume = globalVolume;
      globalVolume = 0;
      return (SB.muted = true);
    }
    //unmute everything
    liveSounds.forEach(function(snd){
      snd.volume = snd.previousVolume;
      delete snd.previousVolume;
    });
    globalVolume = SB.previousVolume;
    return (SB.muted = false);
  };

  // Hide specific soundBatch functionality
  (function(SB){
    function killSound(snd){
      snd.pause();
      snd.onended();
      snd.currentTime = 0;
    }
    function globalVolumeChange(v){ globalVolume = Math.pow(v,2);}
    function globalStop(){liveSounds.forEach(function(snd){killSound(snd)});}
    function globalPause(){ liveSounds.forEach(function(snd){snd.pause();}); }
    function globalGetLive(){return liveSounds.slice();}
    //Applies a callback to anything that passes the filter
    function instanceApply(name, callback){
      var match = sounds[name].src;
      liveSounds.forEach(function(snd){
        if( snd.src !== match ) return;
        callback(snd);
      });
    }

    SB.volume = function(name, v){
      if(!name)return globalVolumeChange(v);
      instanceVolume[name] = Math.pow(v,2);
    };
    SB.stop = SB.kill = function( name ){
      if(!name)return globalStop();
      instanceApply(name, killSound);
    };
    SB.pause = function( name ){
      if(!name)return globalPause();
      instanceApply(name, function(snd){ snd.pause(); });
    }
    SB.getLive = function( name ){
      if(!name)return globalGetLive();
      return liveSounds.filter(function(snd){ return( snd.src === match );});
    }
  })(this.soundBatch);

  var prototypePlay = Audio.prototype.play;//keep a reference

  // Getting  sounds
  this.sound = function(name){
    var snd = new Audio( sounds[name].src );

    snd.volume = instanceVolume[name];
    snd.play = function(){
      var that = this;// used for removing "liveSounds" this conflict.
      that.volume *= globalVolume;
      prototypePlay.apply(that);
      liveSounds.push(that);
      this.onended = function(){ liveSounds.splice( liveSounds.indexOf(that), 1 );};
    };
    return snd;
  };
})();

If you're looking for an ever bigger picture of what this belongs too, it is a part of my Game Framework I started July 4th. Look for "mediaManager.js"

Mainly style comments:

var SB = {muted: false, previousVolume:1}: you should have whitespace after your last colon.
Math.pow(v,2): you should have a space after the comma.
In the following four lines, you should have extra whitespace in between your brackets:

function globalVolumeChange(v){ globalVolume = Math.pow(v,2);}
function globalStop(){liveSounds.forEach(function(snd){killSound(snd)});}
function globalPause(){ liveSounds.forEach(function(snd){snd.pause();}); }
function globalGetLive(){return liveSounds.slice();}

if( snd.src !== match ) return;: you should wrap this in curly brackets, as it can have undesired effects
You should avoid having extra space in your if-else statement brackets like in the point above, and in a few different places.


Custom slice() on list subtype I was working on a subtype of list in python that acts as a list that should never raise an IndexError (every key you input that's greater than the length of the list gets wrapped around). I was having quite a few problems however implementing it properly for slices, who have a LOT of base cases. I wrote the following code to handle slices the way I want to (with example slices commented out);

class Cycle:

    def __getitem__(self, key):
            _self = self._container
            n = len(self)
            if isinstance(key, slice):
                s = key.start
                e = key.stop
                stp = key.step
                r = False
                if stp is None:
                    stp = 1
                elif stp == 0:
                    raise ValueError()
                elif stp < 0:
                    r = True
                    stp = abs(stp)
                if s and not e:
                    sliced = []
                    _len = n - s #[3:]
                    if s < 0:
                        #[-1:]
                        _len = abs(s)
                    elif s >= n:
                        #[7:]
                        _len = n - s % n
                    for i in range(0, _len, stp):
                        index = (i + s) % n
                        sliced.append(_self[index])
                elif e and not s:
                    sliced = []
                    _len = e #[:4]
                    if e <= 0:
                        #[:-1]
                        _len = n - abs(e) % n
                    for i in range(0, _len, stp):
                        index = i % n
                        sliced.append(_self[index])
                elif s and e:
                    sliced = []
                    if s <= e:
                        if s < 0 and e < 0:
                            #[-3:-1]
                            #[-7:-1]
                            _len = abs(s) - abs(e)
                        elif s < 0 and 0 <= e:
                            #[-1:3]
                            #[-7:8]
                            if n + s % n > e:
                                _len = e - s
                            else:
                                _len = e - (n + s)
                        elif 0 <= s and 0 <= e:
                            #[1:3]
                            #[7:19]
                            _len = e - s
                    elif s > e:
                        if s < 0 and e < 0:
                            #[-1:-3]
                            raise ValueError()
                        elif s >= 0 and e < 0:
                            if n + e < s:
                                #[4:-3]
                                raise ValueError()
                            _len = (n + e) - s #[1:-1]
                        elif 0 <= s and 0 <= e:
                            if s >= n:
                                if s % n <= e:
                                    #[7:1]
                                    _len = e - s % n
                                else:
                                    #[7:3]
                                    _len = n - s % n + e
                            else:
                                #[4:2]
                                _len = (n - s % n) + e
                    for i in range(0, _len, stp):
                        index = (i + s) % n
                        sliced.append(_self[index])
                elif not (s or e):
                    sliced = _self[::stp]
                if r:
                    sliced = sliced[::-1]
                return sliced
            else:
                key %= n
                return _self[key]

I can't think of any other more compact way to test for all the different cases that can occur. Can someone help me out please?

I'm not doing a lot of Python but one thing leaping out at me is the usage of all these single letter variables which make the code really hard to read. While longer names will make the code a bit longer I think they will improve readability quite a lot:

s should be start or rangeStart
e should be end or rangeEnd
stp should be step
r: from just briefly browsing over the code I had no idea what that flag is supposed to represent. Given that it's set to True when the step is negative I assume it could mean reverse.


Nested class or two separate classes? I am making this program that is going to calculate distance from randomly generated dots (with x and y coordinates) to 5 bigger ones and then its going to put each smaller dot into the group with bigger one that is nearest to it. So, i am making this with classes and every dot is made as an object. I made 2 arrays of objects. One holds the bigger dots and other one smaller dots. I also made 2 separate classes. I am not sure if this should be done with nested classes/subclasses or my concept is ok.

My classes:

class Groups{
    int x;
    int y;
    int brush; //i use this just for groups to have different color when drawing
    public Groups(int a, int b, int brush)
    {
        this.x = a;
        this.y = b;
        this.brush = brush;
    }
    public int getX()
    {
        return this.x;
    }
    public int getY()
    {
        return this.y;
    }
    public int getBrush()
    {
        return this.brush;
    }
}

public class Dots{
    int x;
    int y;
    int group;

    public Dots(int x, int y)
    {
        this.x = x;
        this.y = y;
        this.group = 0;
    }

    public int getX()
    {return this.x;}

    public int getY()
    {return this.y;}

    public int getGroup()
    {return this.group;}

    public void setGroup(int x)
    {this.group = x;}
}

Naming

Class names in general should be singular - Group, Dot.

If you have a constructor which sets coordinates x and y, call the parameters it gets x and y, not a and b... Also, I would refrain from single letter names, although, in this case, x and y seem ok, since they represent coordinates.

Scoping

You declared members, and implemented getters to them which is fine (although you used the wrong idiom for C#), but you failed to declare the members as private. This means that these members are internal, and can be accessed outside the class:

  Depending on the context in which a member declaration occurs, only
  certain declared accessibilities are permitted. If no access modifier
  is specified in a member declaration, a default accessibility is used.
  Top-level types, which are not nested in other types, can only have
  internal or public accessibility. The default accessibility for these
  types is internal.

Edit - this is, of course, wrong (thanks @Matthew) - I got confused with java), the default access modifier for members in C# is private. I still stand by my recommendation to be explicit with private, to be more readable, and compliant with coding conventions.

Object Oriented

For some reason Dot has a property called Group which is... an int?? What did you intend this property to do? If you want it to hold a Group, its type should be Group:

private Group group;

public Group getGroup() {
    return group;
}

public void setGroup(Group group) {
    this.group = group;
}

I also guess the brush is also an instance of a class (and not an int), and should be declared as such:

private Brush brush;

// exercise for the OP...

Also, what is the role of x and y for Group? I should have guessed that Group will hold some Dots instead...

private List<Dot> dots = new List<Dot>();

public Enumerable<Dot> getDots() {
    return dots;
}

public void addDot(Dot dot) {
    dots.Add(dot);
}

What are nested classes? What are subclasses?

You ask whether you should use subclasses of nested classes, but your example shows neither.

A subclass is a class which inherits from another class, and then looks and behaves just like it, plus some other functionality, which makes it a special case of its parent:

public class ChangeRequest : WorkItem {
    // ....
}

In OO design this is expressed as an is a relationship - "ChangeRequest is a WorkItem"

A nested class is a class which is declared inside another class, it does not inherit anything from it, though it might have special permissions to access its enclosing class:

class Container
{
 public class Nested
    {
        Nested() { }
    }
}

Container.Nested nest = new Container.Nested();

As you can see, the relation between Group and Dot doesn't seem to fall under any of these categories, and they should be written as separate classes, each having a property pointing to the other ('Dot has a Group' and 'Group has Dots').


Counting letters in a text file As a beginner Python programmer, I wrote a simple program that counts how many times each letter appears in a text file. It works fine, but I'd like to know if it's possible to improve it.

def count_letters(filename, case_sensitive=False):
    alphabet = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"

    with open(filename, 'r') as f:
        text = f.read()

    if not case_sensitive:
        alphabet = alphabet[:26]
        text = text.lower()

    letter_count = {ltr: 0 for ltr in alphabet}

    for char in text:
        if char in alphabet:
            letter_count[char] += 1

    for key in sorted(letter_count):
        print(key, letter_count[key])

    print("total:", sum(letter_count.values()))

@flornquake points at the good direction (use string and collections.Counter) but I'd still modify some details:

alphabet = alphabet[:26] and text = text.lower(): My advice is not to override existing variables with new values, it makes code harder to understand. Use different names.
if char in alphabet: Make sure you perform inclusion predicates with hashes, sets or similar data structures, not lists/arrays. O(1) vs O(n).
Functions should return values (hopefully related with their name). Here it makes sense to return the counter.

I'd write:

import collections
import string

def count_letters(filename, case_sensitive=False):
    with open(filename, 'r') as f:
        original_text = f.read()
    if case_sensitive:
        alphabet = string.ascii_letters
        text = original_text
    else:
        alphabet = string.ascii_lowercase
        text = original_text.lower()
    alphabet_set = set(alphabet)
    counts = collections.Counter(c for c in text if c in alphabet_set)

    for letter in alphabet:
        print(letter, counts[letter])
    print("total:", sum(counts.values()))

    return counts


Pass an array of numbers into a method that's overloaded I'm doing my homework and it says to write a test program that prompts the user to enter ten double values, invokes this method, and displays the average value.

I was able to get this program working with an overloaded method... but I'm curious to hear your feedback on where I can improve and what I'm not understanding 100%. Please feel free to review this and let me know your constructive feedback as to my code and where you think I can do better.

import java.util.Scanner;

public class PP68 {

    public static void main(String [] args) {//start main

        Scanner input = new Scanner(System.in); //new object of scanner type

        double avg =0, total =0;

        double[] average = new double[10];      //double array

        for(int i = 0; i < 10; i++){             //enter 10 numbers to average
            System.out.print("Enter a number to average.");
            average[i] = input.nextDouble();
        }

        for(int j = 0; j < average.length; j++) {
            total = total + average[j];
            avg = total / average.length;
        }
        System.out.println("The average of your arrays numbers is: " + avg);

    }//end main

    public static int average(int[] array){ //1st method
        int total =0;
        int avg = total / array.length;
        return avg;

    }//end 1st method
    public static double average(double[] array) {//Second method - overloaded
        double total = 0;
        double avg = total / array.length;
        return avg;

    }//end 2nd method

}

New code follows below:

import java.util.Scanner;

public class PP68v2 {

    public static void main(String [] args) {

    Scanner input = new Scanner(System.in);

    System.out.println("Enter 1 to average an INT or hit 2 to average a DOUBLE");
    int entry = input.nextInt();

        if (entry == 2) {                                  //choose double and call double method
            double[] array = new double[10];
                for(int i = 0; i < 10; i++){
                    System.out.println("Enter a number to average.");
                    array[i] = input.nextDouble();
                }
                System.out.println("The average of your arrays numbers is: " + average(array));

        } else if (entry == 1) {                           //choose int and call int method
            int[] array = new int[10];
                for(int b = 0; b < 10; b++) {
                    System.out.println("Enter a number to average.");
                    array[b] = input.nextInt();
                }
                System.out.println("The average of your arrays numbers is: " + average(array));
        }

    }

    public static int average(int[] array){
        int total =0;
        int avg1 = 0;
            for(int b=0; b < 10; b++) {
                total = total + array[b];
                avg1 = total / 10;
            }
        return avg1;

    }

    public static double average(double[] array) {
        double total = 0;
        double avg = 0;
            for(int i =0; i < 10; i++) {
                total =  total + array[i];
                avg = total / array.length;
            }
        return avg;

    }

}

The code produces the right output, which is good.  It has some problems, though.

The average() functions

As @ChrisW points out, the two average() functions you defined are not being used.  Since your inputs are doubles, you only need one of them: the double average(double[] array) function.

The average() functions are currently buggy:

total = 0, so of course they will each always return 0.  Copy the summation code from main() to fix that.
The int version uses integer arithmetic, so division rounds the result to an integer (towards 0).  In general, the average of a set of integers is not an integer.  You should use double to store the total, even in the integer version.  (You did it correctly in main().)

In case you're wondering, if you want to be able to handle integral and floating-point input arrays, you do need to write out both the int[] and double[] versions explicitly.

main()

If you open a Scanner, it's good etiquette to close it.  The idiom for doing that since Java 7 is to use a try-with-resources block:

try (Scanner input = new Scanner(System.in)) {
    …
}

Avoid declaring and defining variables avg and total until you need them.

The average array is poorly named.  It's an array of ten user input numbers, not an array of ten averages.

You hard-coded 10 twice.  The first for-loop should be more like the second, which uses average.length.

The average only needs to be computed once, not ten times.  Pull the division operation out of the loop.

Of all the comments in the class, the only one worth keeping is // enter 10 numbers to average.  All of the others are redundant noise.


Crop multiple images from single image This code works alright, but it's very difficult to get the dimensions right and takes a bunch of trial and error. I'm wondering how I could improve it. It's meant to cut up board game pieces from a single sheet to then be used for Vassal modules.

It takes a counter sheet (I'll provide a link with an example) and cuts out / exports the individual pieces to be used by another program, which requires each piece to have its own image.

This is an example of a typical countersheet:

############################################
#
#
#       WARGAME COUNTER-CUTTER
#       Programmed by Ray Weiss
#       lerugray@gmail.com
#
#
############################################

#  imports

import os
from PIL import Image
from sys import argv

save_dir = "results"
filename = argv[1]  # Your counter sheet
img = Image.open(filename)

if not os.path.exists(save_dir):
    os.makedirs(save_dir)

coord_input = input("Enter coordinates of first cell x, y: ")
start_pos = start_x, start_y = [int(x) for x in coord_input.split(",")]

counter_input = input("Enter the size of the counters x, y: ")
counter_size = w, h = [int(x) for x in counter_input.split(",")]

major_columns = int(input("Number of Pages: "))
major_columns_w = int(input("Width between cell to cell on the next page: "))
major_rows = int(input("Number of blocks per page: "))
major_rows_h = int(input("height between cell to cell on the next block: "))
columns = int(input("Number of columns inside a block: "))
rows = int(input("Number of rows inside a block: "))

###################################
#
#  End of user supplied arguments
#  Start of program
#
###################################

frame_num = 1

w, h = counter_size

for col_m in range(major_columns):
    for row_m in range(major_rows):
        for row in range(rows):
            for col in range(columns):
                x = w * col + start_x + major_columns_w * col_m
                y = h * row + start_y + major_rows_h * row_m
                crop = img.crop((x, y, x + w, y + h))
                save_at_template = os.path.join(save_dir, "counter_{:03}.png")
                crop.save(save_at_template.format(frame_num))
                frame_num += 1

The images are often off center and unaligned

That is because those images are probably not 100% accurate compared to the user_input.

You could make use of something like opencv to find the individual pieces.
Afterwards you can crop and save those pieces. This will be a big improvement as you could give it a different image, and it would create individual pieces from it... no more measurement from the user!


Simple parsing for string formatting Recently I submitted some string formatting and printing code (found here) that I wrote as an small exercise. It was implemented naively using string replacement. This time around I wanted to actually parse the format string. This is the first time I've ever attempted to write code which does any type of parsing. I'm not 100% sure that this code does not contain any critical flaws. In addition to pointing out any such oversights, I would appreciate advice on how this could be modified to make it either faster or more readable.

The functionality is supposed to be similar to the Console.WriteLine or String.Format functions of C#.

#include <boost/lexical_cast.hpp>
#include <iostream>
#include <string>
#include <exception>
#include <utility>
#include <cctype>

template <typename T>
std::string to_string(T&& item)
{
    return boost::lexical_cast<std::string>(std::forward<T>(item));
}

template <class... Args>
std::string format(const std::string& fmt, Args&&... args)
{
    std::string output;
    output.reserve(fmt.length() * 2);

    std::string arg_strings[] = { to_string(std::forward<Args>(args))... };

    bool placeholder_mode = false;
    std::string index_string;

    for (auto it = fmt.begin(); it != fmt.end(); ++it) {
        //check for end of brace enclosed placeholder
        if ((*it == '}') && (placeholder_mode == true)) {
            if ((index_string.length() == 0)) {
                throw std::invalid_argument("Invalid format string.");
            }
            //check if index_string contains a leading zero
            if ((index_string.length() > 1) && (index_string[0] == '0')) {
                throw std::invalid_argument("Invalid format string.");
            }
            int index = std::stoi(index_string);
            if (index > (sizeof...(args) - 1)) {
                throw std::out_of_range(
                    "Index (zero based) must be greater than or equal "
                    "to zero and less than the size of the argument list."
                );
            }
            output += arg_strings[index];
            index_string.clear();
            placeholder_mode = false;
            continue;
        }

        //escape double closing brace as single
        if ((*it == '}') && (*(it+1) == '}')) {
            it++;
            output.push_back('}');
            continue;
        }

        //check for unescaped closing brace
        //which doesn't close a placeholder
        if ((*it == '}') && (placeholder_mode == false)) {
            throw std::invalid_argument("Invalid format string.");
        }

        //escape double opening brace as single
        if ((*it == '{') && (*(it+1) == '{')) {
            it++;
            output.push_back('{');
            continue;
        }

        //check for begining of brace enclosed placeholder
        if ((*it == '{') && (placeholder_mode == false)) {
            placeholder_mode = true;
            continue;
        }

        if(placeholder_mode == true) {
            //check for non-digit characters
            if (isdigit(*it) == false) {
                throw std::invalid_argument("Invalid format string.");
            }
            index_string.push_back(*it);
        }
        else {
            output.push_back(*it);
        }
    }

    if (placeholder_mode == true) {
        //catch any other placeholders that were left open
        throw std::invalid_argument("Invalid format string.");
    }

    return output;
}

template <class... Args>
void print(const std::string& fmt, Args&&... args)
{
    std::cout << format(fmt, std::forward<Args>(args)...);
}

int main()
{
    print("Hello, {0}! The answer is {1}.\n", "World", 42);
}

Update: Version 2

There are a few points about this that seem open to improvement (at least to me).

Don't compare bools to true or false, just use their value directly, so

placeholder_mode == false

becomes:

!placeholder_mode

likewise:

if(placeholder_mode == true)

becomes:

if (placeholder_mode)

You have basically everything in one giant function. This makes the code more difficult to understand, analyze, etc.
Some of the checks don't seem to accomplish much (if anything)--preventing leading zeros, for one obvious example.
Like most parsers, this is basically a state machine. As in many cases, it would probably be somewhat clearer if the state machine were a little more explicit.

I'd probably start with something like this:

enum states { COPYING, START_PLACEHOLDER, READ_PLACEHOLDER, END_PLACEHOLDER, ERROR };

states state;

while (it != fmt.end()) {
switch (state) {
case COPYING:
    if (*it == '{') state = START_PLACEHOLDER;
    else output.push_back(*it);
    break;
case START_PLACEHOLDER:
    if (*it == '0') state = ERROR;
    else state = READ_PLACEHOLDER;
    // intentional fall-through
case READ_PLACEHOLDER:
        num = std::stoi(it);
        check_range(num, 0, arg_count);
        output.push_back(args[num]);

...and so on. This structure can help clarify what happens in what states, and the conditions under which you progress from one state to another.


Find an ideal speed to cross all traffic lights when they are green As a second exercise with Scala I tried to solve another puzzle.
In this puzzle, we have to find the highest speed that allows a driver to cross all the traffic lights when they are green, without committing speeding.
Each traffic light is defined by its distance from the start and its duration. All the lights start green, stay green during the given duration and then turn red for the given duration, etc.

Example of input:

50       //Max speed for this road
1        //Number of traffic lights on the road
200 15   //Distance and duration for the traffic light

My strategy is to start with the max speed and to try to reach as many traffic lights as possible. If I cross the road of a red light, I calculate the highest speed that allows me to reach it at the beginning of its next cycle (right when it turns green again), then I go back from the start with this new speed.

import math._
import scala.util._

object Solution extends App {

class TrafficLight(distance: Integer, duration: Integer) {

    val fullCycleDuration = 2*duration

    def getIdealSpeed(currentSpeed: Float) : Float = {
        //The number of seconds to go from start to this light
        val timeToReachLight = distance.toFloat / currentSpeed
        //The number of seconds that passed since the light went green
        val timeOfArrivalInCycle = timeToReachLight % fullCycleDuration
        //If we reach the green light in time with this speed, leave it as it is
        if(timeOfArrivalInCycle < duration) {
            currentSpeed
        }
        else {
            //We need to slow down the speed to arrive when it is green again
            val newTravelTime = fullCycleDuration * (timeToReachLight.toInt / Math.round(fullCycleDuration.toInt) + 1)
            //New speed
            distance.toFloat / newTravelTime.toFloat
        }
    }
}

def parseTrafficLight(input: String): TrafficLight = {
    val fields = input.split(" ")
    new TrafficLight(fields(0).toInt, fields(1).toInt)
}

def computeIdealSpeed(currentSpeed: Float, currentId : Integer, allLights: List[TrafficLight]): Float = {
    if(currentId >= lights.length) {
        currentSpeed
    }
    else {
        val newSpeed = lights(currentId).getIdealSpeed(currentSpeed)
        if(newSpeed == currentSpeed)
        {
            computeIdealSpeed(currentSpeed, currentId + 1, allLights)
        }
        else {
            computeIdealSpeed(newSpeed, 0, allLights)
        }
        /*newSpeed match {
            case currentSpeed => computeIdealSpeed(currentSpeed, currentId + 1, lights.tail)
            case _ => computeIdealSpeed(newSpeed, 0, allLights)
        }*/
    }
}

val speedInput = readInt
val speed = speedInput * 1000f / 3600f //km/h to m/s
val lightcount = readInt
var lights = (0 until lightcount).map(_ => parseTrafficLight(readLine)).toList

// Write an action using println
// To debug: Console.err.println("Debug messages...")
val idealSpeed = computeIdealSpeed(speed, 0, lights)

println(Math.round(idealSpeed * 3.6)) //m/s to km/h
}

The code above works correctly but I feel like it could be much better. For example, I wanted to use List.tail for the recursion, but I couldn't find how to get the whole list back when I needed to restart the recursion. This is why I used an id in the recursion. I'd love some suggestions on this part.

I wanted to use List.tail for the recursion, but I couldn't find how to get the whole list back when I needed to restart the recursion.

Easy! You can pass the whole list as an additional parameter.

My Scala is a bit rusty, but I think you can write like this using pattern matching:

def computeIdealSpeed(currentSpeed: Float, lights: List[TrafficLight], allLights: List[TrafficLight]): Float = {
    lights match {
        case light::tail => {
            val newSpeed = light.getIdealSpeed(currentSpeed)
            if (newSpeed != currentSpeed) {
                computeIdealSpeed(newSpeed, allLights, allLights)
            } else {
                computeIdealSpeed(newSpeed, tail, allLights)
            }
        }
        case nil => currentSpeed
    }
}

And then call the method with:

val idealSpeed = computeIdealSpeed(speed, lights, lights)

I'm not sure this is any better though. I would nitpick on the placement of braces and spacing around conditional statements, and the lack of indentation of the body of App.


Finding the most popular element with C# given O(1) space complexity requirement I have tried to solve the following problem in C# with LINQ:

  Given an integer array, find the most frequent number and it's count in the array. If multiple numbers have the same highest frequency return the smallest number.

One requirement here is also to make the code execute in O(1) space complexity.

public struct MostPoularItem
{
    public MostPoularItem(int item, int count)
    {
        Item = item;
        Count = count;
    }

    public int Item { get; }
    public int Count { get; }
}

public static class Utils
{
    public static MostPoularItem FindMostPopularElement(int[] integers)
    {
        if(integers == null)
        {
            throw new ArgumentNullException(nameof(integers));
        }

        return integers.GroupBy(x => x)
            .OrderBy(x => x.Key)
            .OrderByDescending(x => x.Count())
            .Select(x => new MostPoularItem(x.Key, x.Count()))
            .FirstOrDefault();
    }
}

Would be nice to have someone else's eye on this, especially around trying to see if there is any room of improving its time and space complexity. Feel free to offer alternative solutions which don't also use LINQ as well.

You don't have \$O(1)\$ space complexity at the moment, OrderBy and OrderByDescending (used wrong as pointed out in the other answer(s)) will have a non-constant space complexity.

Since runtime is not the problem here, we can make this a space complexity of \$O(1)\$ pretty easily:

Tuple<int, int> GetItemWithMaxCount(int[] items)
{
    var maxCount = 0;
    var maxItem = 0;

    for (int i = 0; i < items.Length; i++)
    {
        var currentCount = 1;
        var currentItem = items[i];

        foreach (var item in items.Skip(i + 1))
        {
            if (item == currentItem)
            {
                currentCount++;
            }
        }

        if (currentCount > maxCount)
        {
            maxCount = currentCount;
            maxItem = currentItem;
        }
        else if (currentCount == maxCount)
        {
            if (currentItem < maxItem)
            {
                maxItem = currentItem;
            }
        }
    }

    return new Tuple<int, int>(maxItem, maxCount);
}

Return an actual class if you like.

The downside of this? Time complexity is \$O(n^2)\$ (I think), but it's guaranteed to use constant space. We also cannot build a lookup of previous iterations, as that would use more than constant space.

Essentially, we just loop through all the items n times, then loop from that item on. This is a trick to help us prevent iterating the same item multiple times. We know that each index can only have a specific value in it so we can skip the that value (and all previous ones) in the inner loop.

There's no complex LINQ here, no additional arrays, no sorting. Just go through them all and add them up then compare to the current max. The .Skip(i + 1) method is part of LINQ, but you can get around that pretty easily by changing that iteration up (swap the foreach out):

for (int j = i + 1; j < items.Length; j++)
{
    if (items[j] == currentItem)
    {
        currentCount++;
    }
}

This can be ordered, unordered, whatever. It meets all your criteria: return item with most counts and count or lowest value item with most counts and count if a tie.

This solution is similar (has the same general effect) to the solution presented in this answer by Peter Taylor, but the key advantage here is that we can optimize it for our requirements. We've already made one optimization: skilling the first i elements. Just as well, that answer will not use constant space because of Select, which appears to use a significant amount of space.

As an example, you can increase speed (slightly) by skipping duplicates as we can with our constant space requirement:

for (int i = 0; i < items.Length; i++)
{
    var currentCount = 1;
    var currentItem = items[i];

    if (currentItem == maxItem)
    {
        continue;
    }

    foreach (var item in items.Skip(i + 1))

Basically, that if will mean that if we are iterating a value again (mind you, it can change, so we can still check the same value twice, but if it's the current max we'll skip it to maintain our space complexity) we can skip it for the moment.

If you read about LINQ-to-objects (which is what we're using) it uses a Stable Quicksort to do it's sorting.

One thing we know about quicksort is that worst-case space complexity is \$O(n)\$ for a naive implementation, and \$O(log(n))\$ for an optimized implementation.

This means that any solution presented that uses LINQ to order the elements is going to use \$O(log(n))\$ space complexity.

And of course, I have to prove that this uses \$O(1)\$ space (or, at least, less space than the OP) so I ran BenchmarkDotNet against my code and OP code:

            Method |            Mean |      StdDev |   Gen 0 | Allocated |
------------------ |---------------- |------------ |-------- |---------- |
      OriginalCode |   1,718.1155 us |   7.6080 us |       - | 327.31 kB |
      EBrownAnswer |  28,547.8824 us |  48.4272 us |       - |     512 B |

Mine took 130 times as long (expected) but ran in 0.15% of the space. So, how do the other answers stack up?

Well, once we put all the answers together (the one by Peter Taylor doesn't return the correct object, but let's just try that one out as well anyway) we get the following:

            Method |            Mean |      StdDev |   Gen 0 | Allocated |
------------------ |---------------- |------------ |-------- |---------- |
      OriginalCode |   1,718.1155 us |   7.6080 us |       - | 327.31 kB |
      EBrownAnswer |  28,547.8824 us |  48.4272 us |       - |     512 B |
  RubberDuckAnswer |   1,183.3647 us |   3.5244 us | 20.3125 | 302.64 kB |
 PeterTaylorAnswer | 255,862.3770 us | 201.6448 us |       - | 760.33 kB |
   HeslacherAnswer |      62.5081 us |   0.3105 us |       - |   8.23 kB |
   PaparazziAnswer |     882.7228 us |   2.9597 us |       - | 157.98 kB |

So there we have it. Looks like the only version that actually had \$O(1)\$ space complexity was the version I posted here. If you can relax that requirement slightly then the version Heslacher posted is the most superior version to use.

If you want a truly optimal solution based off of both metrics of each method (we'll define best as the lowest of time * memoryInBytes), we have the following:

           Method |          Weighted Result |
----------------- |------------------------- |
     OriginalCode |     575,852,937.5283 usB |
     EBrownAnswer |      14,616,515.7888 usB |
 RubberDuckAnswer |     366,728,696.6354 usB |
PeterTaylorAnswer | 199,208,797,290.9158 usB |
  HeslacherAnswer |         526,728.4275 usB |
   PaparazziAnswer|     142,799,409.0947 usB |

The answer posted by Heslacher comes in first, again.

Do note, however, that method is not without it's flaws. As the range between integers grows bigger, the method uses more space. If we're using unbounded int ranges, Heslacher's answer will not work at all. (It will crash on negatives and on extremely large ranges.)

Do note: list generation is not measured for each of these methods and is defined as:

const int _integerCount = 10000;

private static int[] GetItems()
{
    var list = new int[_integerCount];
    var rand = new Random(0);

    for (int i = 0; i < _integerCount; i++)
    {
        list[i] = rand.Next(0, 2048);
    }

    return list;
}

So, if we adjust our integer range to fall within 0 and short.MaxValue (32767), we get the following results:

          Method |           Mean |     StdDev |   Gen 0 |   Gen 1 |   Gen 2 | Allocated |     Result Weighted |
---------------- |--------------- |----------- |-------- |-------- |-------- |---------- |-------------------- |
    EBrownAnswer | 28,408.5030 us | 34.7946 us |       - |       - |       - |     512 B | 14,545,153.5360 usB |
 HeslacherAnswer |    119.7295 us |  0.4404 us | 36.8164 | 36.8164 | 36.8164 | 131.42 kB | 16,112,487.3114 usB |

As this shows, our fastest answer is not always the best. For large ranges of integers (or anything above ~500m, or any values below 0) that solution will fail, sadly. :(

And believe it or not, for small, highly-variable data-sets this algorithm is the fastest (number count = 100, vary from 0 to short.MaxValue):

            Method |       Mean |    StdDev | Scaled | Scaled-StdDev |   Gen 0 |   Gen 1 |   Gen 2 | Allocated |
------------------ |----------- |---------- |------- |-------------- |-------- |-------- |-------- |---------- |
      OriginalCode | 39.0327 us | 0.1790 us |   1.00 |          0.00 |  0.5615 |       - |       - |   9.91 kB |
      EBrownAnswer |  4.6140 us | 0.3984 us |   0.12 |          0.01 |       - |       - |       - |      16 B |
  RubberDuckAnswer | 25.2577 us | 0.0717 us |   0.65 |          0.00 |  0.3499 |       - |       - |   8.39 kB |
 PeterTaylorAnswer | 34.0133 us | 0.1991 us |   0.87 |          0.01 |  0.0651 |       - |       - |   7.76 kB |
   HeslacherAnswer | 30.2370 us | 0.0269 us |   0.77 |          0.00 | 40.3239 | 40.3239 | 40.3239 | 131.42 kB |
   PaparazziAnswer | 17.5162 us | 0.0507 us |   0.45 |          0.00 |  0.5412 |       - |       - |   6.09 kB |


Fetching Python news headlines for Alexa I've recently added a custom Alexa Flask Briefing skill that calls an API endpoint which scrapes top news headlines from the Planet Python RSS feed.

It works - whenever you ask Alexa about the news, it's gonna read top 5 (flash briefing API's limitation) topics from the Planet Python section. But, I'd like to see if there are things to improve in the code:

import logging

from bs4 import BeautifulSoup
from lxml import etree
from dateutil.parser import parse
from flask import Flask, jsonify
from nltk import sent_tokenize
import requests

PLANET_PYTHON_FEED_URL = "http://planetpython.org/rss20.xml"

logger = logging.getLogger()
logger.setLevel(logging.INFO)

app = Flask(__name__)

@app.route("/")
def main():
    with requests.Session() as session:
        session.headers = {
            "User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36"
        }

        response = session.get(PLANET_PYTHON_FEED_URL)
        data = response.content

        root = etree.fromstring(data)

        return jsonify([
            {
                "uid": topic.findtext("guid"),
                "updateDate": parse(topic.findtext("pubDate")).strftime("%Y-%m-%dT%H:%M:%S.%fZ"),  # "2016-04-10T00:00:00.0Z"
                "titleText": topic.findtext("title"),
                "mainText": sent_tokenize(BeautifulSoup(topic.findtext("description"), "lxml").text)[0],
                "redirectionUrl": topic.findtext("link")
            }
            for topic in root.xpath("/rss/channel/item")
        ])

if __name__ == '__main__':
    app.run(debug=True, use_reloader=True)

Note that, since there is a news description length limitation, I had to tokenize the descriptions and cut the first sentences only for the Alexa to read - for that, I've used sent_tokenize from nltk - not sure, if this is an overkill for such a straightforward problem.

Project is also available on github, hosted on Heroku.

You probably want to move the session to a global variable(or simply use requests.get) else at function level it is not helping in re-using any connections right now and is adding an extra level of indentation to your code.
This is mostly opinionated but grouping from x import y based imports below import z makes the imports bit easier to read. Plus keep the items within each group sorted as well.  Google's style guide also contains a related example.

import requests
from bs4 import BeautifulSoup
from dateutil.parser import parse
from flask import Flask, jsonify
from lxml import etree
from nltk import sent_tokenize


Groovy pathfinding algorithm I have created a bit of path-finding code in response to this code golf question. Since this is one of my first large bits of code in Groovy that I have written, I would like to see how I could improve it.

def invalid() {
    println "Invalid map."
    System.exit(1)
}

def map =
        """01001E
010110
000P00
1100J1
S00111""".split('\n')

//map = [""]// TODO remove this, this is type checking only
//map.remove(0)
//reader = System.in.newReader()
//line = reader.readLine()
//while (line != '') {
//    map << line
//    line = reader.readLine()
//}

def size = map[0].size()
map.each { if (it.size() != size) invalid() }

def startLocation = [0, 0]
def endLocation = [0, 0]
map.eachWithIndex { it, idx ->
    def startIdx = it.indexOf('S')
    def endIdx = it.indexOf('E')

    if (startIdx != -1) {
        startLocation[0] = startIdx; startLocation[1] = idx
    }
    if (endIdx != -1) {
        endLocation[0] = endIdx; endLocation[1] = idx
    }
}

def validPath = []

testMove = { x, y, visited ->// visited is an array of x y pairs that we have already visited in this tree
    if (visited.contains([x, y]) || y >= map.size() || x >= map[0].size() || x < 0 || y < 0)
        return;

    def chr = map[y][x]
    def newPath = visited + [[x, y]]

    if (chr == 'E') validPath = newPath
    if ('J0S'.contains(chr) && !validPath) {
        testMove(x - 1, y, newPath)
        testMove(x + 1, y, newPath)
        testMove(x, y + 1, newPath)
        testMove(x, y - 1, newPath)
    }
}
testMove(startLocation[0], startLocation[1], [])
if (!validPath) invalid()

println validPath

def orintation = 0
def path = ''
validPath.inject { first, second ->
    def chr = map[second[1]][second[0]]
    def sub = [first[0] - second[0], first[1] - second[1]]
    def newOrin = [[0, 1]: 0, [1, 0]: 1, [0, -1]: 2, [-1, 0]: 3][sub]
    path += ((((orintation - newOrin) == 0) ? '' : (newOrin > 0 ? 'R' : 'L')) + (chr == 'J' ? 'M' : '') + 'F')
    orintation = newOrin
    second
}
println "path: $path"

I think there are generally two improvements you can make:

Leverage the GDK (probably what you're most interested in)
Refactor the code to make it testable.

First, here's the code:

def map =
"""01001E
010110
000P00
1100J1
S00111""".split('\n')

def path = findPath(map)
println "path: $path"

assert path == 'RFFLFRFMFLFRFLFF'

def findPath(map) {
    /**
     * Validate the map.
     */
    map.with {
        def size = getAt(0).size()
        def invalidRows = it.findAll { row -> row.size() != size }

        if(invalidRows) throw new IllegalArgumentException("The map $it contains the following invalid rows: $invalidRows")

        it.join().split('').countBy { it }.with { counts ->
            if(counts.S > 1) throw new IllegalArgumentException("The map $it contains multiple start positions.")
            if(counts.E > 1) throw new IllegalArgumentException("The map $it contains multiple end positions.")
            if(!counts.S) throw new IllegalArgumentException("The map $it is missing the start position.")
            if(!counts.E) throw new IllegalArgumentException("The map $it is missing the end position.")
        }
    }

    /**
     * Get the start and end locations.
     */
    def (startLocation, endLocation) = { markers ->
        def locations = markers.clone()

        map.eachWithIndex { it, y ->
            markers
                .collectEntries { marker -> [(marker): it.indexOf(marker)] }
                .each { marker, x ->
                    def index = locations.indexOf(marker)

                    if(x != -1) locations[index] = [x: x, y: y]
                }
        }

        return locations
    }(['S', 'E'])

    /**
     * Find the valid path, if possible.
     */
    def validPath = testMove(startLocation.x, startLocation.y, map, [])
    if (!validPath) throw new IllegalArgumentException("The map $map does not contain a valid path.")

    /**
     * Generate the path in the output format.
     */
    def path = validPath.collate(2, 1, false).with {
        def orintation = 0
        def builder = new StringBuilder()

        it.each { pair ->
            def first = pair[0]
            def second = pair[1]
            def chr = map[second[1]][second[0]]
            def sub = [first[0] - second[0], first[1] - second[1]]
            def newOrin = [[0, 1]: 0, [1, 0]: 1, [0, -1]: 2, [-1, 0]: 3][sub]

            builder.append ((((orintation - newOrin) == 0) ? '' : (newOrin > 0 ? 'R' : 'L')) + (chr == 'J' ? 'M' : '') + 'F')
            orintation = newOrin
        }

        builder.toString()
    }
}

def testMove(x, y, map, visited) { // visited is an array of x y pairs that we have already visited in this tree
    if (visited.contains([x, y]) || y >= map.size() || x >= map[0].size() || x < 0 || y < 0)
        return;

    def chr = map[y][x]
    def newPath = visited + [[x, y]]
    def ret

    if (chr == 'E') return newPath
    if ('J0S'.contains(chr)) {
        def locations = [
            [x: x - 1, y: y],
            [x: x + 1, y: y],
            [x: x, y: y + 1],
            [x: x, y: y - 1]
        ]

        for(location in locations) {
            ret = testMove(location.x, location.y, map, newPath)
            if(ret) return ret
        }
    }
}

Leverage the GDK

Early in the script, you have a size variable which is used to ensure all of the rows in the map are of the same length. The size variable is not used elsewhere. So to limit its scope, you can refactor the validation using Object.with(Closure):

map.with {
    def size = getAt(0).size()
    def invalidRows = it.findAll { row -> row.size() != size }

    if(invalidRows) throw new IllegalArgumentException("The map $it contains the following invalid rows: $invalidRows")

    ...
}

In addition, you can validate that the map contains only one start and end position.

map.with {
    ...
    it.join().split('').countBy { it }.with { counts ->
        if(counts.S > 1) throw new IllegalArgumentException("The map $it contains multiple start positions.")
        if(counts.E > 1) throw new IllegalArgumentException("The map $it contains multiple end positions.")
        if(!counts.S) throw new IllegalArgumentException("The map $it is missing the start position.")
        if(!counts.E) throw new IllegalArgumentException("The map $it is missing the end position.")
    }
}

You can assign the startLocation and endLocation directly, without an initial value which is then overwritten, like this:

def (startLocation, endLocation) = { markers ->
    def locations = markers.clone()

    map.eachWithIndex { it, y ->
        markers
            .collectEntries { marker -> [(marker): it.indexOf(marker)] }
            .each { marker, x ->
                def index = locations.indexOf(marker)

                if(x != -1) locations[index] = [x: x, y: y]
            }
    }

    return locations
}(['S', 'E'])

This has the additional benefit of not having side-effects.

Another improvement is to change the location Lists to Maps so that instead of startLocation[0] you can do startLocation.x. This improves readability.

To build the path String, I think using Collection.collate(size, step, keepRemainder) is better suited than Collection.inject(Closure) because it helps clarify the code's intent. inject() can be used on the output of collate(), but that would have been more trouble than it's worth due to the need to keep multiple things in context (accumulator). So I used Object.with(Closure) instead.

def path = validPath.collate(2, 1, false).with {
    def orintation = 0
    def builder = new StringBuilder()

    it.each { pair ->
        def first = pair[0]
        def second = pair[1]
        def chr = map[second[1]][second[0]]
        def sub = [first[0] - second[0], first[1] - second[1]]
        def newOrin = [[0, 1]: 0, [1, 0]: 1, [0, -1]: 2, [-1, 0]: 3][sub]

        builder.append ((((orintation - newOrin) == 0) ? '' : (newOrin > 0 ? 'R' : 'L')) + (chr == 'J' ? 'M' : '') + 'F')
        orintation = newOrin
    }

    builder.toString()
}

In addition, since in the JVM String is immutable, String concatenation can create temporary objects which the JVM must then dispose of. It's typically better to use StringBuilder.

Testability

To make the code testable, meaning that it can be executed via a method call and the output can be validated, I placed most of the code into the findPath() method. As a consequence, I converted the testMove closure into a method.

I know the challenge states If a map is invalid, STDOUT 'Invalid map.' (That includes if the map is unsolvable). However, I disagree with using the same error message for multiple problems. That's one reason I replaced these messages with insightful exceptions. The other reason is to keep the Groovy Console from exiting.


Using BeautifulSoup to scrape various tables and combine in a .csv file A page contains a table of links, each link contains a table relevant to the link (a subject). Create a list of these links to pass to the function called scrapeTable which then takes the table and stores it in a CSV file. A directory of files are created for each subject which are then merged into one master file.

I'm looking for some feedback/criticism/improvements to a piece of code I've written.

import requests
from bs4 import BeautifulSoup
import csv
import pandas as pd
import glob
import os

def scrapeTable(url):
    r = s.get(url)

    soup = BeautifulSoup(r.text,"lxml")

    #get page header
    title = soup.find('h4', 'otherTablesSubTitle')
    subject_name = title.contents[0]

   #get table with 'tablesorter' as name
    table = soup.find('table', {'class': 'tablesorter'})

    #open file using page header
    with open('C:/' + subject_name + '.csv', 'ab') as f:
        csvwriter = csv.writer(f)
        for row in table.findAll('tr'):
            headers = []
            for item in soup.find_all('th'):
                headers.append(item.contents[0])

        #because some pages don't follow exact format, rename any instances of Institution to University
        for idx, h in enumerate(headers):
            if 'Institution' in h:
                headers[idx] = 'University'

        csvwriter.writerow(headers)

        for row in table.findAll('tr'):
            cells = [c.text.encode('utf-8') for c in row.findAll('td')]

            csvwriter.writerow(cells)

    #get third index to use as id for pd.melt
    header_id = headers[2]
    #remove third index to use remaining as values for pd.melt
    headers.pop(2)

    #denormalise the table and insert subject name at beginning
    df = pd.read_csv('C:/' + subject_name + '.csv')
    a = pd.melt(df, id_vars=header_id, value_vars=headers, var_name='Measure', value_name='Value')
    a.insert(0, 'Subject', subject_name)

    a.to_csv('C:/' + subject_name + '.csv', sep=',', index=False)

#details to post to login form
payload = {
    'username': 'username',
    'password': 'password'
}

#use with to close session after finished
with requests.Session() as s:
    p = s.post('websitelogin', data=payload)
    r = s.get('website')

    soup = BeautifulSoup(r.text, "lxml")

    #get list of links (subjects)
    links = []
    for anchor in soup.findAll('a', href=True):
        if 'imported' in anchor['href']:
            links.append('link' + anchor['href'])

    #for each link, call scrapeTable and pass link through
    for item in links:
        scrapeTable(item)

#this merges all the files together into one file called final
path = 'C:/'
allCSV = glob.glob(path + "/*.csv")
frame = pd.DataFrame()
CSVList = []
for file in allCSV:
    df = pd.read_csv(file, index_col=None, header=0)
    CSVList.append(df)

frame = pd.concat(CSVList)
frame.to_csv('C:/final.csv', sep=',', index=False)

Code Style

follow the PEP8 style guide, in particular - fix the imports order, use the lower_case_with_udnerscores variable and function naming style (e.g. scrapeTable and CSVList are violations)

HTML parsing

I think you are over-using the .contents attribute. Consider switching to .get_text()
look into using CSS selectors which is, generally, a more concise way to locate the elements. For example, you can replace table = soup.find('table', {'class': 'tablesorter'}) with table = soup.select_one('table.tablesorter'). Or, you can replace:

links = []
for anchor in soup.findAll('a', href=True):
    if 'imported' in anchor['href']:
        links.append('link' + anchor['href'])

with:

links = ['link' + anchor['href'] for anchor in soup.select("a[href*=imported]")]

where *= means "contains".


Python 3: Why is my prime sieve so slow? I've implemented the sieve of Eratosthenes in Python 3 as follows:

def sieve_of_eratosthenes(n):
    is_prime = [True] * (n+1)
    is_prime[0] = False
    is_prime[1] = False
    p = 0

    while True:
        for i, prime in enumerate(is_prime):
            if prime and i > p:
                p = i
                break
        else:
            break

        multiple = p + p

        while multiple <= n:
            is_prime[multiple]= False
            multiple = multiple + p

    r = []
    for i, prime in enumerate(is_prime):
        if prime: r.append(i)
    return r

Running this to 100,000 takes ~25 seconds.

I felt that was on the slow side, so I decided to take a different approach. The Sieve works prospectivly: it works ahead to clear out all multiples. I made a retrospective function, trial-dividing everything lower than the current prime-candidate:

def retrospect(n):
    primes = [2, 3]
    i = 5

    while i <= n:
        isprime = True
        lim = math.sqrt(n)+1
        for p in primes:
            if p > lim:
                break
            if i % p == 0:
                isprime = False
                break
        if isprime:
            primes.append(i)
        i += 2

    return primes

This is way faster!

Questions:

Are there any obvious shortcomings to the Sieve implementation?
Is there a point where the retrospect gets slower than the sieve?

while True:
        for i, prime in enumerate(is_prime):
            if prime and i > p:
                p = i
                break
        else:
            break

This is the cause of the slowness: track the state between loops and the 25 seconds drop to 0.05 seconds.

        multiple = p + p

        while multiple <= n:
            is_prime[multiple]= False
            multiple = multiple + p

It's probably more Pythonic to use range here:

        for multiple in range(p + p, n + 1, p):
            is_prime[multiple] = False

When handling larger inputs, it becomes worthwhile starting at p * p instead of p + p (on the basis that every composite multiple of p smaller than p * p also has a smaller prime factor).

    r = []
    for i, prime in enumerate(is_prime):
        if prime: r.append(i)
    return r

IMO it makes more sense to inline this into the main loop.

Edited code at this point:

def sieve_of_eratosthenes(n):
    is_prime = [True] * (n+1)
    is_prime[0] = False
    is_prime[1] = False

    primes = []

    for p in range(2, n+1):
        if is_prime[p]:
            primes.append(p)

            for multiple in range(p * p, n + 1, p):
                is_prime[multiple] = False

    return primes

Now if you want more speed at the cost of complicating things ever so slightly, you can apply a simple wheel by special-casing the only even prime:

def sieve_of_eratosthenes(n):
    is_prime = [True] * (n+1)
    primes = [2]

    for p in range(3, n + 1, 2):
        if is_prime[p]:
            primes.append(p)

            for multiple in range(p * p, n + 1, p + p):
                is_prime[multiple] = False

    return primes


struct.unpack on a bytearray that works under Python 2.6.6, 2.7 and 3.3? Given the test code:

import struct
buf = bytearray(b'\xef\xf8\t\xf2')
number = struct.unpack("<I", buf)[0]

How do I cleanly make it work both under Python 2.6 and Python 3.3? The problem is that struct.unpack in Python 2.6 seems to expect a str object (not unicode), so the only way I managed to come up with in order to get it working on both versions is:

buf_raw = bytearray(b'\xef\xf8\t\xf2')
buf_decoded = buf_raw.decode('latin1')
buf_encoded = buf_decoded.encode('latin1')
number = struct.unpack("<I", buf_encoded)[0]

...though I'm abusing the fact that latin1 translates every 0-255 ASCII character to itself. What's the better way?

Why not just call bytes?

number = struct.unpack('<I', bytes(buf))[0]

In Python 2.6:

Python 2.6.8 (unknown, Aug 13 2012, 22:19:05)
...
>>> import struct
>>> struct.unpack('<I', bytes(bytearray(b'ABCD')))
(1145258561,)

In Python 3.3:

Python 3.3.2 (default, May 21 2013, 11:50:47)
...
>>> import struct
>>> struct.unpack('<I', bytes(bytearray(b'ABCD')))
(1145258561,)


C++ calculator using classes This is my first time using classes objects and functions. What can I improve on?

#include <iostream>
#include <conio.h>
using namespace std;

class functions{
public:
    void Body(){
        cout << " :: Welcome to Taylor's CALCULATOR! ::" << endl;
    }
    int Addition(int x, int y){
        int ans = x + y;
        return ans;
    }
    int Subtraction(int x, int y){
        int ans = x - y;
        return ans;
    }
    int Multiplication(int x, int y){
        int ans = x * y;
        return ans;
    }
    int Division(int x, int y){
        int ans = x / y;
        return ans;
    }
};

int main(){
int func;
int x, y;

functions key; //Object
key.Body(); //Object

cout << "What function do you want to use? " << endl;
cout << "1 - Addition " << endl;
cout << "2 - Subtraction " << endl;
cout << "3 - Multiplication " << endl;
cout << "4 - Division " << endl;
cout << "Input: " << endl;

cin >> func;
cout << endl;

switch(func){

    case 1: //Addition
        cout << "**ADDITION**" << endl;
        cout << "Please enter first number: " << endl;
        cin >> x;
        cout << "Please enter second number: " << endl;
        cin >> y;
        cout << x << " + " << y << " = ";
        cout << key.Addition(x, y);
        break;
    case 2: //Subtraction
        cout << "**SUBTRACTION**" << endl;
        cout << "Please enter first number: " << endl;
        cin >> x;
        cout << "Please enter second number: " << endl;
        cin >> y;
        cout << x << " - " << y << " = ";
        cout << key.Subtraction(x, y);
        break;
    case 3: //Multiplication
        cout << "**MULTIPLICATION**" << endl;
        cout << "Please enter first number: " << endl;
        cin >> x;
        cout << "Please enter second number: " << endl;
        cin >> y;
        cout << x << " x " << y << " = ";
        cout << key.Multiplication(x, y);
        break;
    case 4: //Division
        cout << "**DIVISION**" << endl;
        cout << "Please enter first number: " << endl;
        cin >> x;
        cout << "Please enter second number: " << endl;
        cin >> y;
        cout << x << " / " << y << " = ";
        cout << key.Division(x, y);
        break;
    default:
        cout << "Invalid Input...";
        break;
}

}

Unless you'll be maintaining an accumulated number for multiple operations, this may not be the best use of classes.  For your code, you'd get the same effect by just performing these calculations within the switch statement (something similar to this).  Right now, you're just using a class to contain similar functions, including a trivial output function.

Some additional notes:

Try not to use using namespace std.
Indent everything within main() as well.  Not doing so may make it hard to tell that the code is being contained within something.
You don't need <conio.h> here, so just remove it.
It may be useful to prevent the user from dividing by 0, which is undefined behavior.


String-crunching routine How can this be optimized?

char *crunch(char *s)
{
  char *temp,buff[MAX];
  int repeat,count,i,j,k;

  if (*s == '\0')
    return NULL;
  temp = (char*)malloc((strlen(s) + 1) * sizeof(char));
  if (isdigit(*s))
    return NULL;
  repeat = 0;
  for (i = 0, j = 0;i < strlen(s);)
  {
    temp[j++] = s[i++];
    count = 1;
    while (s[i] == s[i - 1])
    {
      count++;
      i++;
      repeat = 1;
    }
    if (isdigit(s[i]) && repeat == 1)
      return NULL;
    if (isdigit(s[i]) && count != 1)
      return NULL;
    else if (isalpha(s[i]) && count != 1)
    {
      temp[j++] = count + 48;
    }
    else if (isdigit(s[i]))
    {
      for (k = 1;k < (s[i] - 48);k++)
        temp[j++] = s[i - 1];
      i++;
    }
  }
  temp[j] = '\0';
  return temp;
}

Examples:

Input: "aabbccdef"
Output: "a2b2c2def"

Input: "a4bd2g4"
Output: "aaaabddgggg"

As written by Jonathan and

Always put a blank line after every }
Always use { } even for single line if/for/while. There is a class of errors where you add a second line in the if but it isn't "in" the if, it is outside. Something like:

if (1 == 1)
   do something

and then you change it in

if (1 == 1)
    do something
    do something else

the do something else isn't in the if, even if you spaced it like it was :-)

If you don't want to use the {} a good style that solves it is to put the second part of the instruction in the same line

if (1 == 1) do something

I don't like it very much (because it makes the line longer to the right) but it is more difficult you make the "bad" error of missing {}

The strlen can be pre-calculated once.

The if (isdigit(*s)) can be put before the malloc.

If you malloc a block of memory and then return NULL you should free the block of memory, otherwise you cause a memory leak.

Please, don't use the pre-fix/post-fix increment operator in the middle of other instructions, they make the code less readable.

This

temp[j++] = s[i++];

is the same as

temp[j] = s[i];
i++;
j++;

but the second is much more readable, because you don't have to think to the order of the operations.

If you have to decompress the string (from a4bd2g4 to aaaabddgggg) the buffer size is too much small.

Considering the compression and decompression are quite different, I would do it in two passes:

first pass: sum all the digits and count all the characters. If the sum is zero the it's compression, do it in function 1. If sum <> 0 it is decompression. With the sum of the digits and the count of the characters you know the size of the buffer you'll need.

If you want to impress your teacher, you should make it work for aaaaaaaaaa (it is 10x a)... It would compress to a9a and aaaaaaaaaaa (11x a) a9a2.


Extracting a set of numbers from a file I do most of my programming in C/C++ and Perl, but I currently learning Fortran. I began by coding up a simple program, something that took me < 5 minutes in Perl (see this thread from BioStar). After working for several hours on a solution in Fortran 95 (teaching myself as I go), I came to this solution.

implicit none

! Variable definitions
integer                 :: argc
integer                 :: num_digits
integer                 :: i, j
integer                 :: iocode
integer                 :: line_length
integer                 :: value
character ( len=256 )   :: infile
character ( len=16 )    :: int_format
character ( len=16 )    :: int_string
character ( len=2048 )  :: line

! Verify and parse command line arguments
argc = iargc()
if( argc < 1 ) then
  write(*, '(A, I0, A)') "Error: please provide file name (argc=", argc, ")"
  stop
endif
call getarg(1, infile)

! Open input file, croak if there is an issue
open( unit=1, file=infile, action='read', iostat=iocode )
if( iocode > 0 ) then
  write(*, '(A, A, A)') "Error: unable to open input file '", trim(infile), "'"
  stop
endif

! Process the file, print in CSV format
do while(1 == 1)
  ! Read the line, skip if it is empty
  100 read( unit=1, fmt='(A)', end=200 ) line
  line_length = len_trim(line)
  if( line_length == 0 ) then
    goto 100
  endif

  ! The first value in the line is a string
  ! Find string boundaries and print it out
  i = 0
  do while( line(i:i) == ' ' )
    i = i+1
  end do
  j = i
  do while( line(j:j) /= ' ' )
    j = j+1
  end do
  write(*, '(A)', advance="no") line(i:j-1)

  ! Now grab the rest of the integer values
  ! on the line, multiply them by 3, and print
  i = j
  j = 0
  do while( i < line_length)
    do while( line(i:i) == ' ' )
      i = i+1
    end do
    j = i
    do while( j < line_length .and. line(j:j) /= ' ' )
      j = j+1
    end do
    int_string = line(i:j-1)
    read(int_string, '(I)') value
    value = value*3
    write(*, '(A, I0)', advance="no") ",", value
    i = j
    j = 0
  end do

  print *
end do
200 close( 1 )

end program

There is a lot that I don't like about how I've written this program, but it's hard to separate my inexperience with the new syntax from bad practice. One thing in particular that I don't like is my use of labels and the infamous goto statement. I'm interested in any feedback whatsoever, but I'm particularly interested in better ways to handle the control structure of the program (without using the goto statement and end parameter in the read function if possible).

The compilers I have access to only support features through f95.

You could get rid of the GOTO statements by using labelled loops with EXIT and CYCLE statements.  These allow you to do the sort of flow control that you've used the GOTOs for, but don't permit the less desirable features, such as computed GOTOs.  Using these newer Fortran features also gives you a clearer idea of the flow control at the point at which the branching occurs.  E.g., CYCLE or EXIT statements send you respectively further up or down the source code, and a well chosen label may indicate what the branching is trying to achieve.

Another suggestion, although this is more of a personal preference, is to avoid the potentially endless DO WHILE(1 == 1) loop by using a normal DO loop with a large upper limit and adding warning message if that limit is reached before the end of file is encountered.  Plenty of people might find that overly fussy though.

Example code showing these two points:

PROGRAM so_goto

  IMPLICIT NONE

  INTEGER,PARAMETER :: line_max = 100000

  INTEGER :: i, ios, line_length
  CHARACTER( len=2048 ) :: line

  OPEN(UNIT=10,FILE="so_goto.txt",ACTION="read")

  fread: DO i=1,line_max
    READ(UNIT=10,FMT='(A)',IOSTAT=ios) line
    IF( ios < 0 ) EXIT fread
    line_length = LEN_TRIM(line)
    IF( line_length == 0 ) CYCLE fread

    IF( i == line_max ) THEN
      print*,"BTW, you've hit your read limit before the end of the file."
    ENDIF
  ENDDO fread
  CLOSE(10)

END PROGRAM so_goto


Refactor method to Java 8 I have a method that iterate a list of maps if a certain element is found then set value of certain variables and break the loop.
I'm trying to revamp this into Java 8. And further if it can be improved, please suggest.

public static void doAction(List<Map<String, String>> decisionTree, String code, StringBuilder destination) {
    boolean conditionFound = false;
    boolean decisionFound = false;
    for (int i = 0; i < 100; i++) {
        if (null != decisionTree) {
            for (Map<String, String> decisionMap : decisionTree) {
                if (null != decisionMap && code.equalsIgnoreCase(decisionMap.get("code"))) {
                    decisionFound = true;
                    String decision = decisionMap.get("is_success");
                    String decisionAction = decisionMap.get("action");

                    BiPredicate<String, String> isApplicable = (d, a) -> "N".equalsIgnoreCase(d) && "Z".equalsIgnoreCase(a);

                    if (isApplicable.test(decision, decisionAction)) {
                        destination.append(decisionMap.get("destination"));
                        conditionFound = Boolean.TRUE;
                    }
                    break;
                }
            }
            if(decisionFound){
                if(conditionFound){
                    // do something
                    break;
                }
            }
        }
    }
}

Probably not the right forum.

In java f(x) the method f will never change the variable x to another value; it cannot be make it null or true or whatever. (A rule to prevent a category of errors.)

So you want a result:

public static class Decision {
    String destination;
    boolean conditionFound;
}

As in java null's should in normal code not be valid, I did not test them.
A test however would not need:

if (null != decisionTree) { // Typical C style.

but can be

if (decisionTree != null) {

As the following is not legal java:

if (decisionTree = null) { // *** Illegal java. // Run-time error in C (assignment).

Searching the code entry result results in an Optional<Decision>.

public static Optional<Decision> doAction(List<Map<String, String>> decisionTree,
            String code) {
    decisionTree.stream()
        .findAny(decisionMap -> code.equalsIgnoreCase(decisionMap.get("code")))
        .map(decisionMap -> {
            Decision decision = new Decision();

            String success = decisionMap.get("is_success");
            String action = decisionMap.get("action");
            BiPredicate<String, String> isApplicable = (s, a) ->
                    "N".equalsIgnoreCase(s) && "Z".equalsIgnoreCase(a);

            if (isApplicable.test(success, action)) {
                decision.destination = decisionMap.get("destination");
                decision.conditionFound = true;
            }
            return decision
        });
}

Optional<Decision> optionalDecision = doAction(decisionTree, code);
optionalDecision.ifPresent(decision -> {
    if (decision.conditionFound) {
        System.out.println(destination);
    }
});

A BiPredicate is not really needed, but not bad.

Instead of a result object, one could also pass an in-out parameter:

public static class DecisionParameter {
    boolean decisionFound;
    StringBuilder destination;
    boolean conditionFound;
}


Testing filtering of certain characters I am a 3rd-year computer science undergraduate. One of my university lecturers has developed his own page for students to submit work. It came up that one student was accused of hacking (sic) by the system. The problem was the characters used in the comment field.

The lecturer claimed that he needed to filter out certain characters in order to stop attacks, and that it was a known problem with PHP. (His system had been broken by students before). I am suspecting poor programming, as he claims that the strings are being executed by PHP.

He told me to try writing a PHP program with a form text field, then input PHP code into the text form. He expects that there to be a vulnerability without filtering the input for bad chars.

<?php

if(isset($_POST['data']) ){
    if( file_exists("../post.sqlite") ){
        $db = new SQLite3("../post.sqlite");

        if( isset($_POST['password']) && $_POST['password'] === "correct horse battery staple" ){
            $statement = $db->prepare("INSERT INTO posts( id, post ) VALUES ( NULL, ? )");
            $statement->bindValue(1, $_POST['data'], SQLITE3_TEXT);
            $statement->execute();
        }

    }
    else {
        $db = new SQLite3("../post.sqlite");

        $db->exec("CREATE TABLE posts( id INTEGER PRIMARY KEY, post TEXT )");
    }
}

function posts(){
    if( file_exists("../post.sqlite") ){
        $db = new SQLite3("../post.sqlite");

        $query = $db->query("SELECT post from posts ORDER BY id DESC");

        while( $row = $query->fetchArray(SQLITE3_ASSOC)){
            print( "
" );
            print( htmlentities( $row['post'] ) );
            print( "

" );
        }
    }
}

?>
<!DOCTYPE html>

    <title>Try This</title>

        <form action="post.php" method="post">
            <textarea rows="10" cols="80" name="data"></textarea>

            <input type="submit" value="Post" />

            Secret key: <input type="text" name="password" />
        </form>

            <?php posts(); ?>

Without the htmlentities function, I know it would have client side vulnerabilities (running malicious scripts). SQL injection is covered (prepared statements). I don't see how the program can be exploited server side. What (if anything) am I missing?

It seems fine for me.

Some small changes:

I'd change the action to

<form action="<?php echo htmlentities($_SERVER['PHP_SELF']); ?>" method="post">

Using PHP_SELF in the action field of a form
I'd consider storing escaped data in the database instead of escaping them on every page requests for performance reasons if the page has lots of visitors.
"../post.sqlite" should be a constant.
I'd reorganize the code a little bit to use only one SQLite3 instance:

define("DB_FILE", "../post.sqlite");

$dbExists = file_exists(DB_FILE);
$db = new SQLite3(DB_FILE);
if (!$dbExists) {
   $db->exec("CREATE TABLE posts( id INTEGER PRIMARY KEY, post TEXT )");
}

if (isset($_POST['data'])) {
    if( isset($_POST['password']) && $_POST['password'] === "correct horse battery staple" ){
        $statement = $db->prepare("INSERT INTO posts( id, post ) VALUES ( NULL, ? )");
        $statement->bindValue(1, $_POST['data'], SQLITE3_TEXT);
        $statement->execute();
    }
}

function posts($db) {
    $query = $db->query("SELECT post FROM posts ORDER BY id DESC");

    while ($row = $query->fetchArray(SQLITE3_ASSOC)) {
        print( "
" );
        print( htmlentities( $row['post'] ) );
        print( "

" );
    }
}

?>
[...]

            <?php posts($db); ?>
[...]

<?php
    $db->close();
?>

It may be worth creating the table with IF NOT EXISTS in every execution instead of the file_exists check for simplicity: Creating an SQLite table only if it doesn't already exist


Menu List and interaction I am wondering if someone can review my code and tell me if there is a way to enhance it and make it more efficient.

import java.util.Scanner;

public class apples {
    public static void main (String[] args){
        Scanner input = new Scanner(System.in);

        int selection;

        for (int i=0; i<4;i++){
            System.out.println("Please enter the selection you would like");
            System.out.println("Enter 1 for Coffee");
            System.out.println("Enter 2 for Tea");
            System.out.println("Enter 3 for Juice");
            System.out.println("Enter 4 for Fizzy Drink");
            selection = input.nextInt();
            if(selection==1){
                System.out.println("You have chosen coffee");
            }
            if(selection==2){
                System.out.println("You have chosen tea");
            }
            if(selection==3){
                System.out.println("You have chosen juice");
            }
            if(selection==4){
                System.out.println("You have chosen fizzy drink");
            }
            if(selection<1 || selection>4){
                System.out.println("You have made a wrong selection, please re-select");
            }
        }
    }
}

Whenever you find you have a situation where you are tring to match values to numbers, especically sequential numbers, the trick is to use an array. This makes the whole thing simpler, and centralizes the logic in one place, and the data in another.

Before we go there, though, some other things are:

Java class names should be Pascal case (upper-case word-starters), so it should be called Apple, and not apple.
There is no need to "pre-declare" variables. int selection; is unnecessary, just make the setting line: int selection = input.nextInt();

For example, your code could easily be:

import java.util.Scanner;

public class Apple {

    private static String[] DRINKS = { "Coffee", "Tea", "Juice", "Fizzy Drink" };

    public static void main(String[] args) {
        Scanner input = new Scanner(System.in);

        for (int i = 0; i < 4; i++) {
            System.out.println("Please enter the selection you would like");

            for (int d = 0; d < DRINKS.length; d++) {
                System.out.printf("Enter %d for %s\n", d + 1, DRINKS[d]);
            }
            int selection = input.nextInt() - 1;
            if (selection >= 0 && selection < DRINKS.length) {
                System.out.println("You have chosen " + DRINKS[selection]);
            } else {
                System.out.println("You have made a wrong selection, please re-select");
            }
        }
    }
}

Note how I have reused the DRINKS array when both printing the prompt, and selecting the Drink. Also note that you can add drinks just by changing the array.

Finally, I find that using printf(...) is convenient when formatting data.


Python to write multiple dataframes and highlight rows inside an excel file I am trying to write multiple dataframes into an excel file one after another with the same logic. Nothing changes for any of the data frame, except the number of columns or number of records. The functions are still the same.

For example,

writer = pd.ExcelWriter(OutputName)
Emp_ID_df.to_excel(writer,'Sheet1',index = False)
Visa_df.to_excel(writer,'Sheet2',index = False)
custom_df_1.to_excel(writer,'Sheet3',index = False)
writer.save()

Now then, once written I am trying to highlight a row if any of the Boolean column has False value in it. There is no library in anacondas that I am aware of can highlight cells in excel. So I am going for the native one.

from win32com.client import Dispatch #to work with excel files
Pre_Out_df_ncol = Emp_ID_df.shape[1]
Pre_Out_df_nrow = Emp_ID_df.shape[0]
RequiredCol_let = colnum_num_string(Pre_Out_df_ncol)
arr = (Emp_ID_df.select_dtypes(include=[bool])).eq(False).any(axis=1).values
ReqRows = np.arange(1, len(Emp_ID_df)+ 1)[arr].tolist()

Pre_Out_df_ncol_2 = Visa_df.shape[1]
Pre_Out_df_nrow_2 = Visa_df.shape[0]
RequiredCol_let_2 = colnum_num_string(Pre_Out_df_ncol_2)
arr_2 = (Visa_df.select_dtypes(include=[bool])).eq(False).any(axis=1).values
ReqRows_2 = np.arange(1, len(Visa_df)+ 1)[arr_2].tolist()

Pre_Out_df_ncol_3 = custom_df_1.shape[1]
Pre_Out_df_nrow_3 = custom_df_1.shape[0]
RequiredCol_let_3 = colnum_num_string(Pre_Out_df_ncol_3)
arr_3 = (custom_df_1.select_dtypes(include=[bool])).eq(False).any(axis=1).values
ReqRows_3 = np.arange(1, len(custom_df_1)+ 1)[arr_3].tolist()

xlApp = Dispatch("Excel.Application")
xlwb1 = xlApp.Workbooks.Open(OutputName)
xlApp.visible = False
print ("\n...Highlighting the Output File at " + datetime.now().strftime('%Y-%m-%d %H:%M:%S'))

for i in range(len(ReqRows)):
   j = ReqRows[i] + 1
   xlwb1.sheets('Sheet1').Range('A' + str(j) + ":" + RequiredCol_let + str(j)).Interior.ColorIndex = 6
xlwb1.sheets('Sheet1').Columns.AutoFit()

for i in range(len(ReqRows_2)):
   j = ReqRows_2[i] + 1
   xlwb1.sheets('Sheet2').Range('A' + str(j) + ":" + RequiredCol_let_2 + str(j)).Interior.ColorIndex = 6
xlwb1.sheets('Sheet2').Columns.AutoFit()

for i in range(len(ReqRows_3)):
   j = ReqRows_3[i] + 1
   xlwb1.sheets('Sheet3').Range('A' + str(j) + ":" + RequiredCol_let_3 + str(j)).Interior.ColorIndex = 6
xlwb1.sheets('Sheet3').Columns.AutoFit()

At last, I am changing the name of the sheet

xlwb1.Sheets("Sheet1").Name = "XXXXA"
xlwb1.Sheets("Sheet2").Name = "XXXXASDAD"
xlwb1.Sheets("Sheet3").Name = "SADAD"
xlwb1.Save()

Now there are a few problems here

1) My number of dataframe increases and which means I am writing up the same code again and again.

2) The highlighting process works but it is too slow. Sometimes 90 % of the rows needs to be highlighted. There are 1 million rows and doing them one after another takes 35 minutes.

Kindly help me with this.

First, starting from your code, you should realize that you are repeating yourself, three times. This goes against the principle Don't repeat Yourself (DRY).

The only real difference between processing your three sheets are their name and the underlying dataframe, so you could make this into two functions:

from win32com.client import Dispatch
import pandas as pd

def highlight_false(df):
    arr = (df.select_dtypes(include=[bool])).eq(False).any(axis=1).values
    return np.arange(1, len(df) + 1)[arr].tolist()

def color_rows(sheet, rows, col):
    for row in rows:
        cells = f"A{row+1}:{col}{row+1}"
        sheet.Range(cells).Interior.ColorIndex = 6
    sheet.Columns.AutoFit()

if __name__ == "__main__":
    Emp_ID_df = ...
    writer = pd.ExcelWriter(OutputName)
    Emp_ID_df.to_excel(writer, 'Sheet1', index=False)

    excel_app = Dispatch("Excel.Application")
    workbook = excel_app.Workbooks.Open(OutputName)
    excel_app.visible = False

    sheet_names = ["Sheet1"]
    dfs = [Emp_ID_df]
    for sheet_name, df in zip(sheet_names, dfs):
        sheet = workbook.Sheets(sheet)
        rows = highlight_false(df)
        col = colnum_num_string(df.shape[1])
        color_rows(sheet, rows, col)

However, there is an even easier method using xlsxwriter:

import pandas as pd

Emp_ID_df = ...

writer = pd.ExcelWriter(OutputName, engine='xlsxwriter')
Emp_ID_df.to_excel(writer, sheet_name="Sheet1", index=False)

workbook = writer.book
format1 = workbook.add_format({'bg_color': '#FFC7CE',
                               'font_color': '#9C0006'})
dfs = [Emp_ID_df]
for df, sheet in zip(dfs, writer.sheets.values()):
    nrow, ncol = df.shape
    col_letter = colnum_num_string(ncol + 1)
    cells = f"A1:{col_letter}{nrow+1}"
    sheet.conditional_format(cells, {"type": "cell",
                                     "criteria": "==",
                                     "value": 0,
                                     "format": format1})
writer.save()

You might have to ensure that the sheets do not get out of snyc from the data frames, or just keep track of what name you save each dataframe to.

In addition I used Python's official style-guide, PEP8, which recommends using lower_case for functions and variables as well as renaming your variables so they are a lot clearer.


Getting Input Key for Atbash Cipher An Atbash cipher is a simple substitution cipher wherein a key is used to determine what letters to swap, for example:

Original alphabet: abcdefghijklmnopqrstuvwxyz
Substitute (key):  zyxwvutsrqponmlkjihgfedcba
Original message:  hello world
Altered message:   svool dliow

In my program, I get the key by printing the original letter, and then getting the letter that is to substitute it, and adding it to a key string, for example:

a = <user input>
b = <user input>
c = <user input>
...

Here is the simple code that I use for it:

std::cout << "Key:\n";
std::string key;

for(int i = 0; i <= 25; i++){
    std::cout << static_cast<char>('a' + i) << " = ";
    char inputChar;

    std::cin >> inputChar;
    key += inputChar;

    std::cin.ignore();
    std::cin.clear();
    std::cout << std::flush;
}

How can I improve this code?

For one thing, it is very inadvisable to use the += operator with strings, because it's slow and it requires the string to be relocated in the memory. This isn't much of a problem with higher-level languages like Python, but with C and also Java, it's not a good idea, and it's not good practice. Since you know that there are 26 letters in the alphabet, you can make a length 26 array to store the key instead. Otherwise, the rest of the code looks good! :)