Parsing Game Entities Could use some help with my presentation in Software Development. I'm presenting how I reworked a game's parser (SpaceTaxi). Basically we take in a .txt file with ASCII characters and convert them into entities that can be rendered in our game.

Input, Output

In between the input and output I've written a parser. I've tried to follow the Facade Design Pattern with the SOLID principles in mind.

I'm not sure how much code I should show here as I don't want to flood you, so I am going to upload some of it but tell me if it's sufficient.

What my code is intended to do

The parser should be a single instance and when called (Parser.StartParse(string txtFile)) the parser should go through a txt file comparing ascii characters with image files.
The image files are given to our project.

IParse.cs

public interface IParse {
    void Parse(string[] txtFile);
    void Add(List<char> map);
}

public class ParseObstacles : IParse {

        public Dictionary<char, string> Dict;
        public List<Entity> Entities;

        public ParseObstacles() {
            Dict = new Dictionary<char, string>();
            Entities = new List<Entity>();
        }

        /// <summary>
        ///     Extracts the txt information about what each character represents.
        ///     Example from the-beach.txt: "A) aspargus-edge-left.png"
        ///     Here we split ')' to first get the character, A, and it's
        ///     correspondent filename, 'aspargus-edge-left.png'.
        /// </summary>
        public void Parse(string[] txtFile) {
            foreach (var i in txtFile) {
                if (i.Contains(")")) {
                    var tile = i.Split(')')[0].ToCharArray()[0];
                    var name = i.Split(')')[1].Trim();
                    Dict.Add(tile, name);
                }
            }
        }

        public void Add(List<char> map) {
            ParseHelper.AddEntities(map, Dict, Entities);
        }
    }

I intended for the parser to follow the facade pattern.
Does this code in fact implement the facade pattern?

First, we encounter interface IParse.

public interface IParse {
    void Parse(string[] txtFile);
    void Add(List<char> map);
}

You should use C# conventions.

interface name should be a noun or noun phrase IParse -> IParser
use a plural name for a variable that represents a sequence of items txtFile -> segments??
use a meaningful name to describe structures map -> ?? -> doesn't say anything
use the correct types, preferrably interfaces, when dealing with sequences; For instance, IEnumerable to iterate, IList to manipulate.
Note that your interface does not have any state or return values. We are never able to interact with it bi-directionally. Too much encapsulation perhaps?

Refactored:

public interface IParser {
    void Parse(IEnumerable<string> segments); // or whatever we are talking about ?
    void Add(IList<char> map); // what is map ?
}

Let's move on to ParseObstacles.

Your state is public, is this as designed or a code smell? Again, use conventions.

public Dictionary<char, string> Dict;
public List<Entity> Entities;

private IDictionary<char, string> values; // they are values I suppose
private IList<Entity> entities;

Next, we find method Parse.

the signature is already changed by interface implementation
Only use i as an index in an iterator
perform i.Split(')') once
Dict is already renamed previously

public void Parse(string[] txtFile) {
    foreach (var i in txtFile) {
        if (i.Contains(")")) {
            var tile = i.Split(')')[0].ToCharArray()[0];
            var name = i.Split(')')[1].Trim();
            Dict.Add(tile, name);
        }
    }
}

After changing the method:

 public void Parse(IEnumerable<string> segments) {
      foreach (var segment in segments) {
          if (segment.Contains(")")) {
              var tokens = segment.Split(')');
              var tile = tokens[0].ToCharArray()[0];
              var name = tokens[1].Trim();
              values.Add(tile, name);
          }
      }
 }

Next up, method Add. This is a black-box for us. Consider the fact ParseHelper adds cyclomatic complexity to your API. Perhaps its code should be divided into the other classes.

public void Add(List<char> map) {
     ParseHelper.AddEntities(map, Dict, Entities);
}

public void Add(IList<char> map) {
       // not convinced about ParseHelper ..
       ParseHelper.AddEntities(map, values, entities);
}

And finally, ParserFacade.

Let's have a look at the singleton.

A private constructor, well done.
A nested static class holding the singleton, good design.
all caps -> take it easy: INSTANCE -> Instance
a getter method is best substituted with a property getter: getInstance() -> Instance

private ParserFacade() { }

private static class ParserFacadeHolder {
    public static readonly ParserFacade INSTANCE = new ParserFacade();
}

public static ParserFacade getInstance() {
    return ParserFacadeHolder.INSTANCE;
}

After changes:

private static class ParserFacadeHolder {
    public static readonly ParserFacade Instance = new ParserFacade();
}

public static ParserFacade Instance => ParserFacadeHolder.Instance;

I don't like this helper class, but I did expect it to be static. Here you used an instance.

  private ParseHelper parseHelper;

Public state detected. Code smell?

 public ParseExits exits;
 public ParseObstacles obstacles;
 public ParsePlatforms platforms;
 public ParseCustomers customers;

Method StartParse:

What bothers me here is the inconsistent and convoluted design of parsing behavior. ParseExits (and sibling classes) can parse the file. But then the facade calls the infamous helper to parse additional data, to then forward this data to ParseExits. This design is too complex in behavioral complexity.

 exits = new ParseExits();
 exits.Parse(file);
 exits.Add(ParseHelper.GetAllTiles(file));


Converting Between Clojure Maps and Lists In an attempt to learn better functional programing I've attempted to convert some Python to idiomatic Clojure, but I'm not sure how well I did with parts of it.

One thing I found myself doing a lot was destructuring a map into a list and then converting it back into a map as in the following example that takes a map of products and ratings (e.g. {:m1 3.5 :m2 2}) and a multiplier and multiplies each rating by the multiplier and returns a map with the corresponding products.

(defn corrected-prod [person multiplyer]
  (into {} (map (fn [[k v]] [k (* v multiplyer)]) person)))

Is there a better way to do this? For context you can reference the code on GitHub.

Here's a few other ways you can go about it:

(defn map-vals [f a-map]
  (zipmap (keys a-map) (map f (vals a-map))))

(defn map-vals [f a-map]
  (reduce-kv #(assoc %1 %2 (f %3)) {} a-map))

or if you're set on transients/into

(defn map-vals [f a-map]
  (persistent! (reduce-kv #(assoc! %1 %2 (f %3)) (transient {}) a-map)))

This is a use case that could maybe stand to have a more elegant core tool, though. Or at least, I'm not the only one to miss it.
For what it's worth, the zipmap approach actually appears to perform best in my cursory testing and is pretty clear in its intent to change the values while keeping the keys as they were. If you're using a function literal it also means that fn doesn't have to take keys it's not going to do anything interesting with, which could make it a little clearer.


Handling Many Unexpected States with except I am creating a server with Flask and it has quite a few routes. Most routes require a specific state, such as "logged in" or "process started". All of my functions and methods in packages will throw certain type of exceptions when the specific state is not matched. At the end my code ends up like this:

@app.route("/sayHello")
def sayHello():
    try:
        name = get_session_data("user", "name")

        # Some other stuff that throw meaningful exceptions

        return someTemplate

    except NotLoggedIn:
        return flask.redirect(flask.url_for("login"))

    except UserNotReadyToSayHello:
        return "You are not ready to say hello!", status.HTTP_400_BAD_REQUEST

    except UserHelloMessageNotDefined:
        return "You did not set up a greeting message!", status.HTTP_400_BAD_REQUEST

    except ConnectionToDatabaseFailed:
        return "Cannot access database", status.HTTP_500_INTERNAL_SERVER_ERROR

    except UnexpectedError:
        return "Something happened", status.HTTP_500_INTERNAL_SERVER_ERROR

Of course this is just an example I made up to demonstrate the problem, however it is more or less equivalent to the actual code.

This made programming the routes much easier than checking everything with if statements everywhere, however I ended up with a lot of except cases to the point where PyLint is telling me

  Too many return statements (9/6)

How should I handle this situation? A lot of routes actually share a very similar sets of except cases.

Handling "too many" return statements.

If your linter tells you, that you have too many return statements, this is a hint. A linter is supposed to  support you in writing quality code.
If however your code needs that specific exception handling this is what you need to implement and you can ignore the linter's hint.
I am not sure whether you can break down the code into several methods, each handling only a part of the possible exceptions, since you decided not to share this essential part of the code with us (# Some other stuff that throw meaningful exceptions).

Re-use of exception handlers

If you want to re-use the exception handlers, you can refactor them into the __exit__ method of an appropriate context manager.
Then you can do something like:

@app.route("/sayHello")
def sayHello():
    with ErrorHandler as error_handler:
        name = get_session_data("user", "name")

        # Some other stuff that throw meaningful exceptions

        return someTemplate

    return error_handler.retval

And re-use the respective context manager ErrorHandler accordingly.


Loop in a .csv file with IF cases I am new to Python and can't quite make my if-cases any shorter.  Any ideas on how to do that?

import csv
fileheader = csv.reader(open("test.csv"), delimiter=",")

# Defines the header of the file opened
header = fileheader.next()

# Loop into the file
for fields in fileheader:

# For each header of the file, it does the following :
# 1/ Detect if the header exists in the file
# 2/ If yes and if the cell contains datas, it's added to the payload that will be used for an HTTP POST request.
# 3/ If not, it doesn't add the cell to the payload.

    if 'url_slug' in header:
        url_slug_index = header.index('url_slug')
        if fields[url_slug_index] == "":
            print "url_slug not defined for %s" % fields[url_index]
        else:
            payload['seo_page[path]'] = fields[url_slug_index]

    if 'keyword' in header:
        keyword_index = header.index('keyword')
        if fields[keyword_index] == "":
            print "keyword not defined for %s" % fields[url_index]
        else:
            payload['seo_page[keyword]'] = fields[keyword_index]

I would consider wrapping the conditions up inside a function:

def is_header(pattern):
    if pattern in header:
        pattern_index = header.index(pattern)
        if fields[pattern_index]:
            pattern_key = "path" if pattern == "url_slug" else "keyword"
            payload['seo_page[{}]'.format(pattern_key)] = fields[pattern_index]
        else:
            print("{} not defined for {}".format(pattern, fields[pattern_index])

Because the tests are essentially the same, you could create a common function and pass in the pattern to test against (url_slug or keyword). There is some extra hocus pocus to get pattern_key to be the right thing in order to point to the right key in your seo_page dict (I assume it is a dict?).


Create GUI components with states I am asking about the right way to make a component that holds some state.  This includes a Jbutton that saves a color in it, or a list item that saves a certain object. So when those GUI components fire an event, I can use the saved states to do something with it.

My way is like this:

Make a subclass of the required component, like a subclass from Jbutton.
Make a Listener for this new subclass: in the listener check, if the event source is the subclass, convert it and then use the stored data.

class ColorButton extends JButton
{
    static class Listener implements ActionListener{

        @Override
        public void actionPerformed(ActionEvent actionEvent) {
            Object source = actionEvent.getSource();
            if( source.getClass() == ColorButton.class)
            {
                ColorButton t = (ColorButton) source;
                t.getComponent().setBackground(t.getColor());
            }
        }
    }

    //states i want to be saved
    private Color c;
    private Component comp;

    ColorButton(Component comp, Color c) {
        setColorChanger(comp, c);
    }

    /*   ......
         ......
         rest of constructors added with those additions
         ......
    */
    private void  setColorChanger(Component comp, Color c)
    {
        this.comp = comp;
        this.c = c;
    }

    Color getColor() {
        return c;
    }

    Component getComponent() {
        return comp;
    }

}

And I use it this way:

JPanel panel = new JPanel();
ColorButton.Listener l = new ColorButton.Listener();
JButton b = new ColorButton("Blue", panel, Color.BLUE);
JButton r = new ColorButton("Red", panel, Color.RED);
r.addActionListener(l);
b.addActionListener(l);
panel.add(b);
panel.add(r);
add(panel);

I am wondering: is this way okay?  I feel it is very boring to make this for every component that should hold a certain state.  Is there a better way?

Instead of comparing the class with if( source.getClass() == ColorButton.class) you can use if (source instanceof ColorButton). This is preferable in case you would make a subclass of ColorButton. If it's an instance of a subclass of ColorButton it is still an instanceof but source.getClass() != ColorButton.class.

With the way that you are using the code above, I would save the color in the listener instead of in the button. This way you don't need the if statement in your code at all, since you can pass the appropriate type to the Listener. In this case, .setBackground is declared in the JComponent class so all you need to make sure is that it's a JComponent that gets passed to the Listener, which is no problem:

static class Listener implements ActionListener {
    private Color color;
    private JComponent comp;
    public Listener(JComponent comp, Color color) {
        this.color = color;
        this.comp = comp;
    }
    @Override
    public void actionPerformed(ActionEvent actionEvent) {
        comp.setBackground(this.color);
    }
}

By using this, you don't need the ColorButton subclass, instead you create multiple instances of your listener:

JPanel panel = new JPanel();
JButton b = new JButton("Blue");
JButton r = new JButton("Red");
r.addActionListener(new Listener(panel, Color.BLUE));
b.addActionListener(new Listener(panel, Color.RED));
panel.add(b);
panel.add(r);
add(panel);


Defining std::vector::push_back(new Object) Is this the best way to construct a vector of 2D points for return?

I don't entirely understand new and delete so I'm concerned about any memory leaks that I might be incurring.

std::vector<Point2D> results;
results.reserve(max_results);

... inside a loop /

Point2D* point = new Point2D(x, y);
results.push_back(*point);
delete point;

It might be that I could do this without constructor.

class Point2D {
public:
    double X;
    double Y;
    Point2D();
    Point2D(double X, double Y);
    void set(double X_p, double Y_p);
    void set(Point2D* point_p);
};

The variable and function names should be lowercase since the types are uppercase.  This would allow for more distinctive naming.
In Point2D, both X and Y should be private.  This applies to all data members.
If you don't need the default constructor for Point2D, leave it out.
There's no need to allocate memory for point; just construct a local object:

Point2D point(x, y);

There should be as little manual memory management in C++ as possible.


Command line flashcard game I've built a simple command line flashcard game for learning Korean. I'm a beginner programmer, so I just wanted to make sure I'm following best practices, especially pertaining to OOP principles.

Any feedback would be greatly appreciated.

Here's GitHub link and reposted below:

class FlashCard

  def initialize
    # TODO: load from file
    @korean_dict = {
      "사과" => "apple",
      "맥주" => "beer",
      "책" => "book",
      "의자" => "chair",
      "피아노" => "piano"
    }
    @english_dict = @korean_dict.invert
    @correct = 0
  end

  # starts the game
  def play
    puts '----------------'
    puts 'KOREAN FLASHCARD'
    puts '----------------'

    dict = pick_dict
    ask_questions(dict)
    print_results(dict)
  end

  # select korean-to-english or english-to-korean dictionary
  def pick_dict
    user_input = -1
    until (user_input == 1 || user_input == 2)
        print "Korean to English [press 1] or English to Korean [press 2]: "
        user_input = gets.chomp.to_i
    end
    user_input == 1 ? @korean_dict : @english_dict
  end

  # loop through dictionary and prompt user for answers
  def ask_questions(dict)
    dict.each_with_index do |(key, val), i|
      print "Question #{i+1}: #{key} is "
      input = gets.chomp
      check_answer(input, val, i)
    end
  end

  # check if user input matches correct translation
  def check_answer(input, val, index)
    if (input == val)
      @correct += 1
      puts "Correct! #{@correct} of #{index+1} correct.\n\n"
    else
      print "Sorry, the correct answer is #{val}."
      print " #{@correct} of #{index+1} correct.\n\n"
    end
  end

  def print_results(dict)
    puts "-------------------------------------"
    puts "Your final score: #{@correct} of #{dict.length}."
    puts "Below is a list of words in the quiz: "
    dict.each { |key, val| puts "#{key} --- #{val}" }
    puts "-------------------------------------\n\n"
  end

end

##################
# Runs Program
##################

fc = FlashCard.new
fc.play

The program's data structures aren't necessarily complicated enough for OOP - you'll almost never need to create multiple instances of any of the potential objects. So don't worry about it too much.

The only big problem I can see is that you always calculate both the Korean-English and English-Korean dictionaries even though only one is used each game, which could be a big waste of memory if the dictionaries get larger. I'd suggest waiting until after the user has chosen their direction and THEN invert the hash if you need to, and ideally throw away the uninverted version (unfortunately Ruby doesn't have an invert! method on hashes). If you're reading from a file, you can invert the data yourself as it is read in.

The one OOP improvement I'd recommend is to wrap the dictionary in an abstract data type. Rather than counting on it being a hash, write methods that return a word to use as a question and the translation of input word, and call those. That way, when you want to make the dictionary bigger or read it from a file, you can just modify those methods. For example, when the dictionary gets really big you might want to read it from disk by random access rather than reading the whole thing into memory. Or you could even write a version that makes requests to a web service.


Printing all even and odd numbers with threads The original question is on careercup.

  Write a multi threaded C code with one thread printing all even numbers and the other all odd numbers. The output should always be in sequence
  ie. 0,1,2,3,4....etc

Now I want to use  C# for it.

class Program
{
    static Object obj = new Object();
    static Thread t1;
    static Thread t2;
    static LinkedList<int> a = new LinkedList<int>();
    static void Main(string[] args)
    {
        for (int i = 0; i < 10; i++)
        {
            a.AddLast(i);
        }

        t1 = new Thread(PrintOdd);
        t2 = new Thread(PrintEven);
        t1.Name = "Odd";
        t2.Name = "Even";
        t1.Start();
        t2.Start();
        t1.Join();
        t2.Join();

        Console.WriteLine("Done!");
        Console.Read();
    }

    private static void PrintOdd()
    {
        while (true)
        {
            if (a.Count == 0)
                break;
            lock (obj)
            {
                int x = a.First();
                if (x % 2 != 0)
                {
                    Console.WriteLine(Thread.CurrentThread.Name + " " + x);
                    a.RemoveFirst();
                }
            }
        }
    }

    private static void PrintEven()
    {
        while (true)
        {
            lock (obj)
            {
                if (a.Count == 0)
                    break;
                int x = a.First();
                if (x % 2 == 0)
                {
                    Console.WriteLine(Thread.CurrentThread.Name + " " + x);
                    a.RemoveFirst();
                }
            }
        }
    }
}

Any improvements?

This looks very good to me, but I am not very advanced at C# myself.  I can give you some tips, though.

First, you should not use ambiguous names like t1 and t2.  You should use more descriptive names like EvenThread and OddThread instead.

Second, you can shorten this:

while (true)
{
  if (a.Count == 0)
    break;

Into this:

while (Numbers.Count > 0)  // For PrintOdd()

And this:

while (Numbers.Count > 1)  // For PrintEven()

Also, you could look into other forms of mutual exclusion techniques, such as semaphores and mutexes, instead of just using an Object.

This is an implementation with a SemaphoreSlim (only code shown is changed):

using System.Threading;

class Program
{
    static SemaphoreSlim ThreadLock = new SemaphoreSlim(1,1);
    static Thread Odd;
    static Thread Even;
    static LinkedList<int> Numbers = new LinkedList<int>();
    static void Main(string[] args)
    {
        for (int i = 0; i < 10; i++)
        {
            Numbers.AddLast(i);
        }

        Odd = new Thread(PrintOdd);
        Even = new Thread(PrintEven);
        Odd.Name = "Odd";
        Even.Name = "Even";
        Odd.Start();
        Even.Start();
        Odd.Join();
        Even.Join();

        Console.WriteLine("Done!");
        Console.Read();
    }

    private static void PrintOdd()
    {
        while (Numbers.Count > 0)
        {
            ThreadLock.Wait();

            int x = Numbers.First();
            if (x % 2 != 0)
            {
                Console.WriteLine(Thread.CurrentThread.Name + " " + x);
                Numbers.RemoveFirst();
            }

            ThreadLock.Release();
        }
    }

    private static void PrintEven()
    {
        while (Numbers.Count > 1)
        {
            ThreadLock.Wait();

            int x = Numbers.First();
            if (x % 2 == 0)
            {
                Console.WriteLine(Thread.CurrentThread.Name + " " + x);
                Numbers.RemoveFirst();
            }

            ThreadLock.Release();
        }
    }
}


Throwing an exception within a try/catch block in PHP Is there a cleaner way to do this?

    try {
        $ffprobe      = FFProbe::create();
        $streams      = $ffprobe->streams($infile);
        $videoStreams = $streams->videos();
        $audioStreams = $streams->audios();

        if (!$videoStreams) {
            throw new Exception("Could not find a video stream for file: " . $infile . PHP_EOL);
        }

    } catch (RuntimeException $e) {
        throw $e;
    }

The way you did it is pretty much the standard way to bubble exceptions up the stack. Basically something like:

function a(){
    throw new Exception("Exceptional!");
}

function b(){
    try{
        a();
    } catch (Exception $e){
        throw $e;
    }
}

try{
    b();
} catch (Exception $e){
    echo $e->getMessage();
}

This will output:

Exceptional!

Do note that if you in b() do throw new Exception($e), you'll get:

exception 'Exception' with message 'Exceptional!' in sandboxed.php:1
Stack trace:
    #0 php(5): a()
    #1 php(12): b()
    #2 {main}

For your code however, you'd have to make sure that you are your try/catch block is within another try/catch block, otherwise you will end up with an uncaught exception error.

try{
    try {
        $ffprobe      = FFProbe::create();
        $streams      = $ffprobe->streams($infile);
        $videoStreams = $streams->videos();
        $audioStreams = $streams->audios();

        if (!$videoStreams) {
            throw new Exception("Could not find a video stream for file: " . $infile . PHP_EOL);
        }

    } catch (RuntimeException $e) {
        throw $e;
    }
} catch (Exception $e){
    echo $e->getMessage();
}

May I ask why you are bubbling exceptions like this rather than erroring out on a caught exception?

EDIT
Fail often:

try{
    $ffprobe = FFProbe::create();

    try {
        $streams = $ffprobe->streams($infile);
    } catch (RuntimeException $e) {
        throw $e;
    }

    $videoStreams = $streams->videos();
    $audioStreams = $streams->audios();

    if (!$videoStreams) {
        throw new Exception("Could not find a video stream for file: " . $infile . PHP_EOL);
    }
} catch (Exception $e){
    echo $e->getMessage();
}


Hibernate MassIndexer using chained invocation for building a batch job I've a class called MassIndexer.java (on GitHub) and it uses chained invocation for building a batch job. Each method assigns a property of this job and finally the job will be started using method MassIndexer#start(). Here's an example for Java EE:

long executionId = new MassIndexer()
        .addRootEntities(Company.class, Employee.class)
        .start();

According to different environments, the requirement is different. Some methods are required in Java SE while all of them are optional in Java EE. If users use MassIndexer under Java SE and they forget to set the properties through those required methods, the job will fail. These required methods on Java SE are:

isJavaSE(boolean)
entityManagerFactory(EntityManagerFactory)
jobOperator(JobOperator)

Here's another example for Java SE:

long executionId = new MassIndexer()
        .addRootEntities(Company.class, Employee.class)
        .isJavaSE(true)
        .entityManagerFactory(emf)
        .jobOperator(jobOperator)
        .start();

However, the design of this class in not clear: people don't know which method is required and which isn't. The only way to know it is to read the documentation. And I'm wondering how can I improve the design of this class, so that people can easily understand what to do and how to do. For example, should this class be split into MassIndexerEE and MassIndexerSE? Should it be refactored into a builder class, e.g. BatchIndexingJobBuilder?

And there's no order or logic among these methods. You can call foo() then bar(), but bar() then foo() works too. Properties are used for configurations based on

selection of class types (about persistence)
database interaction (about persistence)
parallel processing (about job)
checkpoing algorithm (about job)

Someone must be confused when using this class, so I'm seeking advice.  Every review is welcomed.

Make a second level Builder class

long executionId = new MassIndexer()
        .addRootEntities(Company.class, Employee.class)
        .javaSE()
        .entityManagerFactory(emf)
        .jobOperator(jobOperator)
        .start();

long executionId = new MassIndexer()
        .addRootEntities(Company.class, Employee.class)
        .javaEE()
        .start();

class MassIndexer
    MassIndexer addRootEntities(Class<?>...)
    SEBuilder javaSE()
    EEBuilder javaEE()

class MassIndexer.SEBuilder
    long start()
    SEBuilder entityManagerFactory(EntityManagerFactory)
...

I am not that happy with the name and using a constructor for MassIndexer
addRootEntities might go in the "constructor"
javaSE/EE maybe standalone/server or container?


JavaFX controller for a poker game I am making a poker application in JavaFX. When I try to display the cards on the screen it is working perfectly fine. Yet I have a method with a lot of duplicated code in it.

I have tried to compress the code into 1 function with a for loop but I couldn't get it to work. Anyone has an idea how to get rid of the duplication?

public class GameController implements Initializable {
    @FXML
    private Canvas canvasBoard;
    @FXML
    private Canvas canvasPlayer1;
    @FXML
    private Canvas canvasPlayer2;
    @FXML
    private Canvas canvasPlayer3;
    @FXML
    private Canvas canvasPlayer4;

    private int playerCardsWidth1 = 0;
    private int playerCardsWidth2 = 0;
    private int playerCardsWidth3 = 0;
    private int playerCardsWidth4 = 0;
    private int dealerCardsWidth = 0;
    private TexasHoldem game;
    private IPlayer board;

    public GameController() {
        try {
            game = new TexasHoldem();
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

    private void showCardsBoard() {

        board = game.getBoard();

        dealerCardsWidth += 50;
        for (ICard c : board.getCards()) {
            String foto = c.getFilename();
            Image image = new Image(foto);
            GraphicsContext gcBoard = canvasBoard.getGraphicsContext2D();
            int dealerCardsHeight = 0;
            gcBoard.drawImage(image,dealerCardsWidth, dealerCardsHeight,60,80);
            dealerCardsWidth += 75;
        }
    }

    private void showCardsPlayers() {

        List<IPlayer> spelers = game.getPlayers();

        for (int i = 0; i < spelers.size(); i++) {

            if (i == 0) {
                IPlayer player1 = spelers.get(i);
                for (ICard c : player1.getCards()) {
                    Image image = new Image(c.getFilename());
                    GraphicsContext gc1 = canvasPlayer1.getGraphicsContext2D();
                    int playerCardsHeight1 = 0;
                    gc1.drawImage(image, playerCardsWidth1, playerCardsHeight1, 60, 80);
                    playerCardsWidth1 += 75;
                }

            }
            if (i == 1) {
                IPlayer player2 = spelers.get(i);
                for (ICard c : player2.getCards()) {
                    Image image = new Image(c.getFilename());
                    GraphicsContext gc2 = canvasPlayer2.getGraphicsContext2D();
                    int playerCardsHeight2 = 0;
                    gc2.drawImage(image, playerCardsWidth2, playerCardsHeight2, 60, 80);
                    playerCardsWidth2 += 75;
                }
            }
            if (i == 2) {

                IPlayer player3 = spelers.get(i);
                for (ICard c : player3.getCards()) {
                    Image image = new Image(c.getFilename());
                    GraphicsContext gc3 = canvasPlayer3.getGraphicsContext2D();
                    int playerCardsHeight3 = 0;
                    gc3.drawImage(image, playerCardsWidth3, playerCardsHeight3, 60, 80);
                    playerCardsWidth3 += 75;
                }
            }
            if (i == 3) {

                IPlayer player4 = spelers.get(i);
                for (ICard c : player4.getCards()) {
                    Image image = new Image(c.getFilename());
                    GraphicsContext gc4 = canvasPlayer4.getGraphicsContext2D();
                    int playerCardsHeight4 = 0;
                    gc4.drawImage(image, playerCardsWidth4, playerCardsHeight4, 60, 80);
                    playerCardsWidth4 += 75;
                }
            }
        }
    }

Logic:

If statements differs only in 2 variables: canvasPlayerX and playerCardsWidthX.

playerCardsWidthX: looks like this var can be converted to local, just like playerCardsHeightX;

canvasPlayerX: you can do one little trick here - add player canvases to a list in initialize() method, and then refer to the i-th player canvas as playersCanvases.get(i);

Moreover, your code didnt even compile, because GameController should implement initialize(..) method;

Codestyle:

playerCardsWidthX - in your code, its not width of player cards, its coordinate, where to draw next card. Consider renaming;
in constructor, exception is caught, and stacktrace is printed, but after that, program will continue its flow. Is this what you want? (see code below for possible solution).
extract constants into private static final fields

public class GameController implements Initializable {
public static final int CARD_WIDTH = 60;
public static final int CARD_HEIGHT = 80;
public static final int CARD_SPACING = 15;

public static final int CARDS_X_DIFF = CARD_WIDTH + CARD_SPACING;

@FXML
private Canvas canvasBoard;
@FXML
private Canvas canvasPlayer1;
@FXML
private Canvas canvasPlayer2;
@FXML
private Canvas canvasPlayer3;
@FXML
private Canvas canvasPlayer4;

private List<Canvas> playersCanvases;

private TexasHoldem game;
private IPlayer board;

public GameController() {
    try {
        game = new TexasHoldem();
    } catch (Exception e) {
        e.printStackTrace();
        Platform.exit();
    }
}

@Override
public void initialize(URL url, ResourceBundle resourceBundle) {
    // in initialize method, because in constructor canvases are not assigned(== null)
    playersCanvases = Arrays.asList(canvasPlayer1, canvasPlayer2, canvasPlayer3, canvasPlayer4);
}

private void showBoardCards() {
    board = game.getBoard();
    showCards(50, board.getCards(), canvasBoard.getGraphicsContext2D());
}

private void showPlayersCards() {
    List<IPlayer> players = game.getPlayers();
    for (int i = 0; i < players.size(); i++) {
        IPlayer player = players.get(i);
        showCards(0, player.getCards(), playersCanvases.get(i).getGraphicsContext2D());
    }
}

private void showCards(int xOffset, List<ICard> cards, GraphicsContext gc) {
    int cardY = 0;
    int cardX = xOffset;
    for (ICard c : cards) {
        Image image = new Image(c.getFilename());
        gc.drawImage(image, cardX, cardY, CARD_WIDTH, CARD_HEIGHT);
        cardX += CARDS_X_DIFF;
    }
}

}


Populating an array with times with half hour interval between them So for example, getTimes("12:00", "15:30"); should return:
["12:00", "12:30", "13:00", "13:30", "14:00", "14:30", "15:00", "15:30"]

The input parameters are always in :30 or :00 intervals(so 06:00, 06:30, 07:00, ...).
getTimes("06:00", "17:00"); -> should work

getTimes("06:30", "17:00"); -> should work

getTimes("06:25", "17:00"); -> doesn't have to work because the from or until time can never be "06:25"

It also only has to work for the range 00:00 - 23:30 the same day.

What I have right now is this:

function getTimes(from, until){
            //"01/01/2001" is just an arbitrary date
            var until = Date.parse("01/01/2001 " + until);
            var from = Date.parse("01/01/2001 " + from);
            //*2 because because we want every 30 minutes instead of every hour
            var max = (Math.abs(until-from) / (60*60*1000))*2;
            var time = new Date(from);
            var hours = [];
            for(var i = 0; i <= max; i++){
                //doubleZeros just adds a zero in front of the value if it's smaller than 10.
                var hour = this.doubleZeros(time.getHours());
                var minute = this.doubleZeros(time.getMinutes());
                hours.push(hour+":"+minute);
                time.setMinutes(time.getMinutes()+30);
            }
            return hours;
}

How can this be improved?

Naming (more later)

I'd recommend renaming your function to something more specific than getTimes to avoid collisions with other getTimes functions. A more specific name also provides more information on what the function does.

function createHalfHourIntervals() {}

Looser Coupling

Another potential improvement would be to decouple your time formatting from your time interval creating. You could use a sleek functional programming technique to easily add the formatting after you've made the intervals:

function createHalfHourIntervals(from, until){
  // ...
  var time = new Date(from);
  var intervals = []; // more clear name than hours

  for (var i = 0; i <= max; i++) {
    intervals.push(time);
    time.setMinutes(time.getMinutes() + 30);
  }
}

function formatDateHHcolonMM(date) {  // funny name but specific
  var hour = date.getHours();
  var minute = date.getMinutes();
  return doubleZeros(hour) + ":" + doubleZeros(minute);
}

var intervals = createHalfHourIntervals()
  .map(formatDateHHcolonMM); // map will create a new array of formatted intervals from the array returned by createHalfHourIntervals

This separates your formatting logic from your interval creation logic, making your code more reusable and cohesive. If another programmer were less familiar with your code, but wanted to change the formatting of your intervals in someway, that task is made much simpler in this format. Each section of code doing one task, in general, can make the project easier to understand. However, if you don't plan on reusing much of the code in getTimes or working with other developers, sometimes it can just be simpler to keep all the code contained in one single location.

Good Optional Ideas

Although you may not want it now, it's possible that you may want to create arrays of intervals of different lengths in the future. Again, this depends on what you actually need, but 20min intervals or even interval times offset by 5min [7:05, 7:35] can be quite common. You could prepare better for the future change by reworking your function a bit. Your "01/01/2001 ", "*2" and "+30" are what some consider magic numbers, which tend to make the code less readable--sometimes even requiring their own comments. A good variable name oftentimes is better than a comment, so you may want to give these numbers their own variable names, improving clarity while preparing for a future refactoring:

function createHalfHourIntervals(from, until) {
  var arbitraryDay = "01/01/2001 ";
  // ...

  var intervalLength = 30;
  var intervalsPerHour = 60 / intervalLength;
  var milisecsPerHour = 60 * 60 * 1000;
  // are you sure that Math.abs(until - from) is the behavior you want. It seems like preparation to allow the user to put the 'from' and 'until' parameters in the other order.
}

That's pretty much all of your code! Hope you find this helpful, and keep up the good work!


Detect strings that consist of some a's followed by some b's This program needs to detect strings in which the first few characters are 'a', than few 'b'.
Is it possible that in this implementation may be undefined behavior.

string str;
while (cin >> str) {
    unsigned int i = 0;
    while (str[i] == 'a' && i < str.length()) {
        i++;
    }
    while (str[i] == 'b' && i < str.length()) {
        i++;
    }
    if (i == str.length()) {
        cout << "ok" << endl;
    } else {
        cout << "Not ok" << endl;
    }
}

So:

// Note: str is a string.
//       containing "a"
//       So length = 1

while (str[i] == 'a' && i < str.length()) {
    i++;
}
// First Loop
//     str[0] == 'a' && 0 < 1
//     true && true
//     true
//       So we enter the loop body and increment i to 1.
// Second Loop
//     str[1]  // access beyond the end of the string.
//             // thus undefined behavior.

If we swap the test around

while (i < str.length() && str[i] == 'a') {
    i++;
}
// First Loop
//     0 < 1 && str[0] == 'a'
//     true && true
//     true
//       So we enter the loop body and increment i to 1.
// Second Loop
//     1 < 1  &&  // The left hand side of the test is false
//                // && is shortcut operator so the right hand
//                // side is not evaluated and the loop exited.


JavaScript and RxJS Password Strength Indicator This is my first time using RxJS (or any observable library). It seems to work okay but I'm not sure about best practices.

The thing that I am most unsure about is subscribing to the observable multiple times. I couldn't decide if it would be better to subscribe to it once with one big function, or to use many smaller subscribers.

CodePen Demo

Markup

  Password

    <input id="password" class="input" type="password" />

  Password Strength
  <progress id="passwordStrength" class="progress is-danger" value="0" max="5" />

Script

const { fromEvent } = rxjs
const { map, filter } = rxjs.operators

document.addEventListener('DOMContentLoaded', () => {
  const passwordInput = document.getElementById('password')
  const passwordHelp = document.getElementById('passwordHelp')
  const passwordStrength = document.getElementById('passwordStrength')

  const passwordChanged$ = fromEvent(passwordInput, 'input')
    .pipe(
      map(x => x.target.value),
      map(zxcvbn)
    )

  passwordChanged$
    .pipe(map(password => password.score > 2 ? 'success' : 'danger'))
    .subscribe(state => {
      passwordInput.className = `input is-${state}`
      passwordStrength.className = `progress is-${state}`
    })

  passwordChanged$
    .subscribe(password => { passwordHelp.innerHTML = password.feedback.warning })

  passwordChanged$
    .subscribe(password => { passwordStrength.setAttribute('value', password.score + 1) })
})

Each subscribe(...) invocation results in creating and returning a new Subscription unless special measures are taken (like .publish(), .share()...)

  Subscribing to observables

  Remember, observables are lazy. If you don’t subscribe nothing is going to happen. It’s good to know that when you subscribe to an observer, each call of subscribe() will trigger it’s own independent execution for that given observer. Subscribe calls are not shared among multiple subscribers to the same observable. — Luuk Gruijs

Each Subscription

eats up a bit of resources;
is processed separately from the others;
and needs to be unsubscribed separately to release the resources.

Therefore, if you have no special reasons, you should subscribe the least number of times. In your case, a single subscription is sufficient. You can still use separate functions to delegate them the actual work (setClassNames(), setWarning(), setStrength()).

const { fromEvent } = rxjs
const { map, filter } = rxjs.operators

document.addEventListener('DOMContentLoaded', () => {
  const passwordInput = document.getElementById('password')
  const passwordHelp = document.getElementById('passwordHelp')
  const passwordStrength = document.getElementById('passwordStrength')

  const passwordChanged$ = fromEvent(passwordInput, 'input')
    .pipe(
      map(x => x.target.value),
      map(zxcvbn)
    )

  const setClassNames = password => {
    const state = password.score > 2 ? 'success' : 'danger';
    passwordInput.className = `input is-${state}`
    passwordStrength.className = `progress is-${state}`
  };
  const setWarning = password => { passwordHelp.innerHTML = password.feedback.warning };
  const setStrength = password => { passwordStrength.setAttribute('value', password.score + 1) };

  passwordChanged$
    .subscribe(password => {
      setClassNames(password);
      setWarning(password);
      setStrength(password);
    });
})


Filter one array based on values of another, is this the best way? categories = [{id: "761601bc-4daf-4db2-a0cf-fe7f443fcb94", name: "Shoes"},{id: "601ebcfe-fcbd-4075-a4b4-a42e356c5747", name: "Hats"},{id: "238933cf-77a5-4443-b290-7d7c836f80ff", name: "Eyewear"}];

voucher = {id:1,categories:["761601bc-4daf-4db2-a0cf-fe7f443fcb94","601ebcfe-fcbd-4075-a4b4-a42e356c5747"]};

filterCategories(voucher) {
  const result = voucher.categories
    ? categories
        .filter(cat => {
          return voucher.categories.includes(cat.id);
        })
        .map(v => v.name)
        .sort()
        .join()
    : "";

  return result;
}

Expecting: "Hats,Shoes"

The result needs to be filtered and sorted and returned as a string. Is there a faster, more efficient way of doing this. Maybe using Lodash?

Normally there are a lot of categories, so I'd go through the voucher.categories array:

const filterCategories = (voucher, categories) => (
  Array.isArray(voucher.categories) ?
    voucher.categories
      .map(categoryId => {
        const categoryFound = categories.find(c => c.id === categoryId);
        return categoryFound ? categoryFound.name : null;
      })
      .filter(categoryName => categoryName !== null)
      .sort()
      .join() :
    ""
);

This way you don't have to go through the entire list of 'master' categories (which in practice may be very long) doing a search on the voucher categories in each iteration.

Note: it would be much faster if you could have the categories as an object with the id as index:

categories = {
  ["761601bc-4daf-4db2-a0cf-fe7f443fcb94"]: {name: "Shoes"},
  ["601ebcfe-fcbd-4075-a4b4-a42e356c5747"]: {name: "Hats"},
  ["238933cf-77a5-4443-b290-7d7c836f80ff"]: {name: "Eyewear"}
};

const filterCategories = (voucher, categories) => (
      Array.isArray(voucher.categories) ?
        voucher.categories
          .map(categoryId => {
            const categoryFound = categories[categoryId];
            return categoryFound ? categoryFound.name : null;
          })
          .filter(categoryName => categoryName !== null)
          .sort()
          .join() :
        ""
    );


Tail program output to stdout or stderr I have a piece of bash code that works fine.
for thing in "$things"; do
    output=$(my_program "$thing" 2>&1)

    if [[ $? -eq 0 ]]; then
        echo "$(echo "$output" | tail -1)"
    else
        echo "$(echo "$output" | tail -1)" 1>&2
    fi
done

The idea is to echo the last line of the output from the subcommand to stdout if it worked and to stderr if it didn't work.
Is there a better way to achieve this?

Don't quote $things:
for i in "1 2 3 4"; do
   echo "i=$i"
done
i=1 2 3 4
# Better is
for i in 1 2 3 4; do
   echo "i=$i"
done
i=1
i=2
i=3
i=4

When my_program is written well, errors are written to stderr and normal output to stdout. Your bash code is working well, so the program already uses the right returncode.
Can you change the my_program? When you can add an option -s (silent/summary), the program can make sure only 1 line is written to stdout (when OK) or stderr (when NOK):
for thing in ${things}; do
    my_program -s "${thing}"
done

Or without for-loop
printf "%s\n" ${things} | xargs -L1 -I{} my_program -s "{}"

When you can't change my_program, or you don't want to, move the special handling to a function and remove the additional echo commands:
tail_my_program() {
   output=$(my_program $* 2>&1)
   if (( $? == 0 )); then
      tail -1 <<< "${output}"
   else
      tail -1 <<< "${output}" >&2
   fi
}

for thing in ${things}; do
    tail_my_program "${thing}"
done

Your original solution and my function have a small bug: When my_program writes something to stdout after writing an error message to stderr, the stdout message is selected after an exit 1. This might never happen.


Loading BitmapImages out of a Zip file I'm working on a personal project to get better with C#, and as part of that I have a zip file with 20 or so images saved inside. I want to load those images out of the zip file and display them in my WPF app. I have it working, but I don't know if what I have is the best way to go about this. The goal is to load the images to be used as fast as possible, so I tried using threads, which work, but again I'm not sure if this is the proper way to utilize them for this, or if threads are even the best way to achieve the quickest time for the task.

I would appreciate any feedback anyone could give to help me improve this code and write better code in the future.

In the code below, I have the two functions I use to open and get the images. the variable ChapterLocation is a string that stores the file location of the .zip file, and Pages is a List. Page is a custom struct that just holds a BitmapImage and a String that gives that image a name that I can display.

private void BitmapLoad(ZipArchiveEntry z)
{
    using (var stream = z.Open())
    {
        using (var mstream = new MemoryStream())
        {
            stream.CopyTo(mstream);
            mstream.Position = 0;

            var bitmap = new BitmapImage();
            bitmap.BeginInit();
            bitmap.CacheOption = BitmapCacheOption.OnLoad;
            bitmap.StreamSource = mstream;

            bitmap.EndInit();
            bitmap.Freeze();

            Pages.Add(new Page(bitmap, z.Name));
        }
    }
}

private void LoadChapterOptimized()
{
    var zipFile = ZipFile.OpenRead(ChapterLocation);
    foreach (ZipArchiveEntry z in zipFile.Entries)
    {
        Thread t = new Thread(() => BitmapLoad(z));
        t.Start();
        t.Join();
        Debug.WriteLine(z.Name);
    }
    Pages.Sort();
    zipFile.Dispose();
}

var zipFile = ZipFile.OpenRead(ChapterLocation);
    ...
    zipFile.Dispose();

One word: using.

    using (var stream = z.Open())
    {
        using (var mstream = new MemoryStream())
        {
            stream.CopyTo(mstream);
            mstream.Position = 0;

            var bitmap = new BitmapImage();
            bitmap.BeginInit();
            bitmap.CacheOption = BitmapCacheOption.OnLoad;
            bitmap.StreamSource = mstream;

            bitmap.EndInit();
            bitmap.Freeze();

This needs a comment explaining why mstream is necessary and you can't just use stream.

I suspect that really mstream is either unnecessary or insufficient. If it's necessary then it's probably because of threading issues due to multiple ZipArchiveEntry streams all reading from one backing stream, and in that case doing the read with stream.CopyTo might be reducing the probability of the problem surfacing rather than actually fixing it.

    foreach (ZipArchiveEntry z in zipFile.Entries)
    {
        Thread t = new Thread(() => BitmapLoad(z));
        t.Start();
        t.Join();
        Debug.WriteLine(z.Name);
    }

Ah, it's not really multi-threaded after all. If you want to do old-school multithreading then you need to spin off all of the threads before calling Join().

I say "old-school" because a more modern approach would be to use Parallel Linq. Bearing in mind my previous comment about streams, the approach would probably be to map the entries to (string Name, MemoryStream ms)s in one thread and then map those tuples to Pages in parallel threads. I think that this can be written as

Pages = zipFile.Entries.
    Select(z => Read(z)).
    AsParallel().
    Select(tuple => LoadPage(tuple)).
    OrderBy(page => page).
    ToList();

but I haven't really used Parallel Linq myself, and I don't guarantee that this does the Read calls all in the same thread.


Anagrams for a given input 2.0 This question follows on from :Previous Question

Here is what my reworked code looks like. I neatened it up as much as I could. Changed some of the semantics of it. I also tried to add some faster exit points and checks to prevent errors.

Any further critiques?

 /**
 * @author              :KyleMHB
 * Project Number       :0002
 * Project Name         :Anagramatic
 * IDE                  :NETBEANS
 * Goal of Project      -
 * Capture user input, compare to a dictionary file for anagrams,
 * output number of matches and the matches.
 */

package anagramatic;

import java.io.File;
import java.io.FileNotFoundException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.Scanner;
import javax.swing.JOptionPane;

-

public class Anagramatic {
public static void main(String[] args) throws FileNotFoundException{
    String anagram=getInput("Enter the word you would like to process");
    List<String> words=readAnagramsFromFile(anagram, new File("words.txt"));
    String output= formatOutput(anagram,words);
    displayOutput(output);
}//pvsm

-

private static String getInput(String prompt) {
    String input = JOptionPane.showInputDialog(null,prompt);
    return input;
}//getInput

-

  private static List readAnagramsFromFile(String word, File f)
        throws FileNotFoundException{
    ArrayList<String> anagrams = new ArrayList<>();
    try(Scanner s = new Scanner(f)){
        while(s.hasNext()){
            String candidate=s.next();
            if ( (candidate.length()==word.length()) &&
                    (checkMatch(word,candidate)==true)){
                anagrams.add(candidate);
            }
        }
}
    return anagrams;
}//readFile

-

private static boolean checkMatch(String word, String candidate) {
    char[] wordArray = word.toCharArray();
    char[] candidateArray = candidate.toCharArray();
    if (Arrays.equals(wordArray, candidateArray)){
        return false;
    }
    Arrays.sort(wordArray);
    Arrays.sort(candidateArray);
    if(Arrays.equals(wordArray, candidateArray)){
        return true;
    }
    /**I did not use an else function for the the below return
    *because if (Arrays.equals(wordArray, candidateArray))==true
    *it will break on the return*/
    return false;

}//match

-

private static String formatOutput(String original, List<String> words) {
    StringBuilder output=new StringBuilder("[ ");
    int counter=0;
    Iterator<String> wordIt =words.iterator();
    while(wordIt.hasNext()){
       output.append(wordIt.next());
       if(wordIt.hasNext()){
           output.append((++counter % 8 == 0)? ",\n" : ", ");
       }
    }
    output.append(" ]");
    return ("The Anagram "+original+" has "+words.size()+" matches.\n\nThey are:\n"+output.toString());
}//formatOutput

-

private static void displayOutput(String output){
    JOptionPane.showMessageDialog(null,output);
}//displayOutput
}

Four issues with private static boolean checkMatch(String word, String candidate):

checkMatch is not as descriptive as a name could be.  I think private static boolean isAnagram(String a, String b) would be better.  I would also rename the parameters to acknowledge that they are symmetrical: it doesn't matter which is the original word, and which is the candidate.
You don't need to end with an if-else.  You can just say return Arrays.equals(wordArray, candidateArray);
You would be re-sorting the characters of the original word each time you test a candidate.  That's OK, I suppose, if you prefer clean code over efficiency.
Why not start with

private static boolean isAnagram(String a, String b) {
    if (a.length() != b.length()) {
        return false;
    }

    // Continue with a thorough comparison...
    ...
}

Then the code in readAnagramsFromFile() can read very smoothly:

if (isAnagram(word, candidate)) {
    anagrams.add(candidate);
}

Note that the return type of readAnagramsFromFile() should be List<String>, not just a generic List.


Initial attempt at node.js automatic redirection I'm working on a Node.js HTTP server on my iPhone:

var fs = require("fs");
var http = require('http');
const port = 8080;
var readFile = function(name, response) {
    var read = function(err, dat) {
        var rv = null;
        var nope = "./redirect.html";
        if (err) {
            if (name == nope)
                rv = "INTERNET!";
            else {
                rv = readFile(nope, response);
                return;
            }
        }
        else rv = dat;
        response.end(rv);
    }
    fs.readFile(name, "utf-8", read);
}
var client = function(request, response) {
    path = "." + request.url;
    file = readFile(path, response);
}
var server = http.createServer(client);
server.listen(port);

redirect.html:

<script language = "javascript">
window.location = "/index.html";
</script>
Bad request.

if (err) {
        if (name == nope)
            rv = "INTERNET!";
        else {
            rv = readFile(nope, response);
            return;
        }
    }
    else rv = dat;

Omitting {} can easily go sideways especially if you mix it like in the example above. I suggest to always use them.

Even if it's just

else { rv = dat; }

they greatly helps to see where the if/else ends.

var nope = "./redirect.html";

Since you doesn't seem to change it anwhere, I'd make it a constant too.


Merge sort 2 sorted linked list My coding skill is pretty rusty and moreover, I have never paid much attention to writing elegant code. I want to start on this. Problem is to merge 2 sorted linked list. Algorithmically this is trivial, but looks a good problem to practice coding.

struct node
{
 char *data;
 struct node *nextp;
}

typedef struct node node;
typedef struct node list; //I do 2 typedefs here to signify when conceptually I
//mean a whole list and when just a single node.is this a good practice? or only
//one that confuses.

node * add(node *listp, char *data)
{
 node * newp=(node *)malloc(sizeof(node));
 assert(newp != NULL);
 newp->data=(char *)malloc(sizeof(strlen(data));
 assert(newp->data != NULL);
 strcpy(newp->data,data); // allocate space in newp->data . should there be +1?
 newp->next=NULL;
 if(listp==NULL)
  listp=nextp ;
 else
  listp->nextp=newp;
 return newp;
}

list *mergelist(list * list1p, list *list2p)
{
// initialize mergedlistp;
node dummy;
node* mergedlistendp=&dummy;
list* leftlistp = NULL;

if (list1p == NULL) leftlistp=list2p;
if(list2p == NULL) leftlistp = list1p;

if(list1p != NULL && list2p!= NULL)
{
while(1)
{

 if(strcmp(list1p -> data,list2p -> data) <=0)
 {
  mergedlistendp=add (mergedlistendp,list1p->data);
  list1p=list1p -> next;
  if(list1p == NULL)
  {leftlistp=list2p; break;}
 }
 else
 {
  mergedlistendp=add (mergedlistendp,list2p->data);
  list2p=list2p -> next;
  if(list2p == NULL)
  {leftlistp=list1p; break;}
 }

}

for(leftlistp!=NULL;leftlistp = leftlistp->next)
add(mergedlistendp,leftlistp->data);

return mergedlistendp; // I have to return mergedlistp here (i.e. head of list)
//. How to store address of mergedlistendp the first time it is assigned a value?
}

I would like my code to be reviewed.Any feedback on naming, program flow, correctness, better code, indenting, idioms etc is appreciated. Please advise on how corner cases are handled more elegantly.

You should have added error checking:

 node * newp=(node *)malloc(sizeof(node));
 newp->data=(char *)malloc(sizeof(strlen(data));

malloc can return NULL if there wont be enough Virtual Adress Space. Would be great if function return error code.

add {} it could bite your next time

  if(listp==NULL)
   listp=nextp ;
  else
   listp->nextp=newp;
   //next live goes here, looks like next statment in else, but it's not.

Use PascalCase or camelCase

// initialize mergedlistp;
node* mergedlistendp=NULL; // mergedList is a better name
list* leftlistp = NULL;

Stick to one style, once you have:

if (list1p == NULL) leftlistp=list2p;
if(list2p == NULL) leftlistp = list1p;

another one

  if(listp==NULL)
   listp=nextp ;

or

  if(list2p == NULL)
   {leftlistp=list1p; break;}

One statment per line

 if(list2p == NULL)
 {leftlistp=list1p; break;} // put break in the next line.


Using readonly field instead of private setters in struct This is supposed to be an immutable structure representing a time-stamped value. Since it represents a physical measurement, it cannot change, and I want to make sure it really doesn't change accidentally at runtime.

public struct Sample
{
    public TimeSpan Time { get { return _time; } }
    private readonly TimeSpan _time;

    public Double Value { get { return _value; } }
    private readonly Double _value;

    public Sample(TimeSpan time, Double value) : this()
    {
        _time = time;
        _value = value;
    }
}

I have heard a lot about "properties are better than fields", and it's common to see the following idiom:

public Double Foo { get; private set; }

But I wonder:

Does it make a difference? I feel I prefer the current, field-based form, but maybe I am missing something...
In case it does, which one is preferrable, regarding immutability (and also other characteristics)?

Thought I'd chime in:

You've stated that this should be an immutable struct. With that in mind, the solution you want is the one with the backed readonly fields.

I feel explicitly stating intent in code is good. You say that you want immutability, then write the code so.

If you were to opt for the property only solution then consider what would happen if some other developer came along and misunderstood your original intent (because this time it wasn't fully explicit)

With the properties you are stating - The public consumers of my struct may not mutate it but the struct itself can.

With the readonly fields you are stating - The public consumers of my struct may not mutate it and neither may the struct itself.

The latter is correct in regards to your spec.

To briefly summarise:

"Does it make a difference?" Yes, your intent is different between the two. Yes, one allows mutation after the point of declaration, one does not.

"In case it does, which one is preferrable, regarding immutability (and also other characteristics)?"
For immutability, the readonly option.


JavaScript dragon slayer game (Codecademy) I'm currently doing the Javascript course at Codecademy, and decided to customize their dragon slayer game. I would appreciate any help/pointers that can help me optimize the code. There's a lot of repetition that I'm sure can be done in a smarter way.

var slaying = true;
var youHit = Math.floor(Math.random() * 2);
var totalDamage = 0;
var playerHp = 100;
var dragonHp = 100;

function dragonSlayer() {
    while(slaying) {
        var damageThisRound = Math.floor(Math.random() * 50 + 1);
        console.log("Player HP: " + playerHp);
        console.log("Dragon HP: " + dragonHp);
        if(youHit) {
            totalDamage += damageThisRound;
            console.log("You did " + totalDamage + " damage!");
            console.log(" ");
            dragonHp -= totalDamage;
            if(dragonHp <= 0) {
                console.log("Player HP: " + playerHp);
                console.log("Dragon HP: 0");
                console.log("YOU WIN!");
                slaying = false;
            } else {
                totalDamage = 0;
                youHit = Math.floor(Math.random() * 2);
            }
        } else {
            totalDamage += damageThisRound;
            console.log("The dragon did " + totalDamage + " damage!");
            console.log(" ");
            playerHp -= totalDamage;
            if(playerHp <= 0) {
                console.log("Player HP: 0");
                console.log("Dragon HP: " + dragonHp);
                console.log("YOU ARE DEAD!");
                slaying = false;
            } else {
                totalDamage = 0;
                youHit = Math.floor(Math.random() * 2);
            }
        }
    }
};

dragonSlayer();

Added benefit of function

The function does not seem to have any added benefit right now. For example, running the function twice will not print two "fights". This is mainly because you use global variables. The function is useful if this was a class, where you can set and reset the health of entities. In this case I would just pass them as parameters.

Performance

Performance is about the speed at which the code executes. I do not see flaws which should hamper execution speed too much.

DRY

Your code can be optimized by noticing that the if and the else-statement are nearly identical. You can optimize it by extracting that code and making that it's own function.

Now identify that the log-lines with hp for each entity are duplicated 3 times, twice to tell if we won or not. We can do better. Let's just do it once, and break out of the loop with break when we detect a final conclusion.

The function now does not much useful. We can just put those 2 lines in the first function itself.

For more information, see Wikipedia

Weird damage calculation

totalDamage += damageThisRound;
console.log("You did " + totalDamage + " damage!");
console.log(" ");
dragonHp -= totalDamage;

What is this supposed to do? You reset totalDamage every time, except in a winning/losing condition in which case this code is never executed again.

Variable hoisting

Javascript has a feature called variable hoisting. All variables defined in a context are moved to the top of that context before any other code is executed in that context. I recommend to move all declarations to the top of the context to better match what the code does.

Improvement

function slayDragon( playerHp, dragonHp ) {
  var playerTurn;
  var damageThisTurn;

  while(true) {
    console.log("Player HP: " + Math.max(playerHp, 0));
    console.log("Dragon HP: " + Math.max(dragonHp, 0));

    //End conditions
    if(playerHp <= 0) {
      console.log("YOU ARE DEAD!");
      break;
    } else if(dragonHp <= 0) {
      console.log("YOU WIN!");
      break;
    }

    //Who gets to swing?
    playerTurn = Math.floor(Math.random() * 2);
    damageThisTurn = Math.floor(Math.random() * 50 + 1);

    if( playerTurn ) {
      console.log("You do " + damageThisTurn + " damage.");
      dragonHp -= damageThisTurn;
    } else {
      console.log("Dragon does " + damageThisTurn + " damage.");
      playerHp -= damageThisTurn;
    }

    //Spacing
    console.log("");
  }
}

slayDragon(100, 100);


Add getters and setters to a set of variables Any improvements are welcome.

a.bat

sed -r "s/private (.*) (.*);(.*)/&\npublic \1 get\u\2(){return \2;}\npublic void set\u\2(\u\2){this.\2=\u\2;}\n/" "%~1">"%~2"

in:

private String firstName;// Stores firstName
private String lastName;
private String address;

using a.bat in out

out:

private String firstName;// Stores firstName
public String getFirstName(){return firstName;}
public void setFirstName(FirstName){this.firstName=FirstName;}

private String lastName;
public String getLastName(){return lastName;}
public void setLastName(LastName){this.lastName=LastName;}

private String address;
public String getAddress(){return address;}
public void setAddress(Address){this.address=Address;}

To do:
add javadoc generation to methods.

You forgot the parameter type in the setter methods (including @blufox suggestion):

sed -r "s/private\s+([^; ]+)\s+([^; ]+)\s*;(.*)$/&\npublic \1 get\u\2(){return \2;}\npublic void set\u\2(\1 \2){this.\2=\u\2;}\n/" "%~1">"%~2"

which produces:

private String firstName;// Stores firstName
public String getFirstName(){return firstName;}
public void setFirstName(String firstName){this.firstName=FirstName;}

But: you better use an IDE (e.g., Eclipse) for these tasks. You are trying to parse Java code with a regex which is a bad idea.

Just some examples:

declarations could be on different lines

private String // some comment
    myVariableOnTheNextLine
    // some other comment
    ; // semicolon

the public or private modifier could be omitted

String myString;

fields can be initialized

String myString = "my string";

fields can be declared final

private final String myString;

and so on


Generating packing slips depending on product type My abstract class. Product will have multiple child classes those will be different types of Product
 public abstract class Product
            {
                private int productId;
                public int ProductId
                {
                    get
                    {
                        return productId;
                    }
                    set
                    {
                        productId = value;
                    }
                }

                private double cost;
                public double Cost
                {
                    get
                    {
                        return cost;
                    }
                    set
                    {
                        cost = value;
                    }
                }
                protected IPayment payment;
                protected IEmail email;
                public string DoPayment()
                {
                    return payment.MakePayment();
                }

                public void setDifferentPaymentMethod(IPayment payment)
                {
                    this.payment = payment;
                }
            }

Concrete implementation of Product Class . How can I avoid instantiating PackingSlip here. Shall I create a factory?
 public class PhysicalProduct:Product
    {
        public PhysicalProduct()
        {
            payment = new PackingSlip();
        }
    }

Concrete implementation of Product Class
public class BookProduct:PhysicalProduct
    {
        public BookProduct():base()
        {
            payment = new DuplicatePackingSlip();
        }
    }

Strategy class
 internal class DuplicatePackingSlip : IPayment
    {
        public string MakePayment()
        {
            return "Generating Duplicate Packing SLip for Royalty Department";
        }
    }

Strategy.
 internal class PackingSlip : IPayment
        {
            public string MakePayment()
            {
                return "Generating Packing Slip";
            }
        }

Is this the right way to implement if we want to have open/closed principle?

Indentation
Yours is highly inconsistent. It's best to use an autoformatter to ensure that this remains sane and consistent.
Property boilerplate
This:
            private int productId;
            public int ProductId
            {
                get
                {
                    return productId;
                }
                set
                {
                    productId = value;
                }
            }

is both very Java and unnecessary given the syntactic sugar that C# has supported for several years:
public int ProductId { get; set; }

Factory?

How can I avoid instantiating PackingSlip here

Why do you want to avoid instantiating it? Is it because PhysicalProduct is supposed to be an abstract class as well? If so, you are correct to want to avoid assigning an instance in the abstract class, since your assignment in the BookProduct constructor will overwrite this value.
There are several approaches you can follow. One is to make the top-level constructor accept an instance of IPayment, and to pass a new instance via base().
Another approach is to make the abstract base class a generic, where the generic type is forced to implement IPayment. The abstract class would not need to accept a new instance, and would be able to construct its own instance so long as the new constraint is imposed.
Redundant base call
This:
    public BookProduct():base()

should not need to explicitly call base.
MakePayment
One of two things has happened here. Either you've omitted a bunch of code, which would technically make this section of the question off-topic; or MakePayment is both misnamed and mischaracterized as a function. If the latter, make it a get-only property and rename it to something like Description.


Is it still necessary to check for a front camera? I'm trying to clean up redundant old code, so I'm asking this general question. Since all iOS 7 devices have a front camera, would it be safe to remove the code to check if the device has a front camera? In the past this was necessary as not all devices with iOS 6 had a front camera.

It is true that Apple could theoretically release a new device without a front camera, but I doubt this would ever happen.

What is your answer though on what I should do?

BOOL deviceHasFrontCamera = NO;

NSArray *videoDevices = [AVCaptureDevice devicesWithMediaType:AVMediaTypeVideo];

for (AVCaptureDevice *device in videoDevices)
{
    if (device.position == AVCaptureDevicePositionFront)
    {
        deviceHasFrontCamera = YES;
    }
}

According to the iOS Device Compatibility Reference, the iPhone 3GS was the only iOS 6-capable device to lack a front camera.

The same document states that your application can declare the requirement for a front-facing camera by setting the UIRequiredDeviceCapabilities key such that front-facing-camera is true.  That way, you can safely strip out the camera-checking code while having your application self-document the assumption that the front camera exists.


Simple Ruby directory navigator functions In the process of writing scripts to work with CSVs and other files of the like I found myself in the need of a way to navigate and select files for various purposes. I wrote the following function for one of the scripts I needed to finish quickly and now I'm trying to get it into a more reusable form

def select_file()
  i = 1
  selection_set = Dir.glob("*")
  puts "--------------------------------------------------------------------------"
  puts "Listing entries in #{File.basename(Dir.pwd)}:"
  puts
  selection_set.each do |entry|
    puts i.to_s + " - " + entry
    i = i+1
  end
  print "Select File/Folder: "
  user_selection = gets.chomp
  selection = selection_set[user_selection.to_i - 1]
  puts "Selected: #{selection}"
  puts "--------------------------------------------------------------------------"
  selection
end

In an attempt to make this more reusable in the future I refactored this single function into a series of functions. Part of the motivation in doing it this way was to attempt to use this for displaying other sets of things the user may need to select. Was I better off sticking with the original approach or am I on a decent track but not quite doing it right? I get the sense this can all be done much simpler. Also I am deliberately avoiding using any kind of UI gem for no reason other than I am trying to learn.

def full_directory_navigator
  selection_set = Dir.glob('*', File::FNM_DOTMATCH)
  list_directory_entries(selection_set)
end

def dir_header(pattern)
  100.times { |i| !(i == 99) ? print('-') : puts }
  pattern_message = (pattern == '*') ? '' : "matching \"#{ pattern }\""
  puts
  puts "Listing entries in #{ File.basename(Dir.pwd) }#{ pattern_message }"
  puts
  puts 'Index          Entry'
  100.times { |i| !(i == 99) ? print('-') : puts }
end

def dir_select
  print 'Select File/Folder: '
end

def spacer(i)
  size = i.to_s.size
  spacer = ''
  (9 - size).times { spacer = ' ' + spacer }
  spacer
end

def display_selection_set(selection_set)
  i = 1
  selection_set.each do |entry|
    puts i.to_s + spacer(i) + '-     ' + entry
    i += 1
  end
end

def list_directory_entries(selection_set, pattern = '*')
  dir_header(pattern)
  display_selection_set(selection_set)
  dir_select
  user_selection = gets.chomp.to_i
  selection = selection_set[user_selection - 1]
  puts "Selected: #{selection}"
  selection
end

full_directory_navigator

Any and all comments are appreciated. If my code sucks please let me know so I can start writing less awful code. I also just realized I didn't comment the code at all, so I will begin commenting it and if there are any ambiguous sections it would be greatly appreciated if it could be brought to my attention.

Your two code samples are not exactly equivalent.  The second one has nicer formatting (fixed-width numbering) and attempts to parameterize the glob pattern (but fails in that regard — full_directory_navigator should accept a pattern parameter and pass it to both Dir.glob and list_directory_entries).

I'm not convinced that splitting up the select_file function into six functions makes it any better.  It's not any more readable or extensible than the original.  The parameter passing just gets in the way.  (If you had defined a class, there might be some advantage, in that a subclass could customize the behaviour by overriding one of the methods.)

With more effective string formatting, the code is simple enough to fit in one function.

To repeat a character, use string multiplication.
The ternary conditional could just be an if-qualified expression, since one of the outcomes is nil.
To make a fixed-width column of numbers, use '%-8d' % number.

def select_file(glob='*', glob_flags=File::FNM_DOTMATCH)
  entries = Dir.glob(glob, glob_flags)

  # Header
  puts '-' * 79,
       "Listing entries in #{File.basename(Dir.pwd)}#{
                               " matching \"#{glob}\"" if glob != '*'
                           }",
       '',
       '%-8s   %s' % ['Index', 'Entry'],
       '-' * 79

  # Listing
  entries.each_with_index { |entry, i| puts '%-8d - %s' % [i + 1, entry] }

  # Prompt
  print 'Select File/Folder: '
  entries[gets.to_i - 1]
end

selection = select_file()
puts "Selected: #{selection}"

Note that you haven't done any validation on the input.  If the input doesn't look like a number, then gets.to_i - 1 becomes 0 - 1, which refers to the last item in the array.


Arp-spoof check in PowerShell To learn some PowerShell I made a tool, I've made before in bash. It pings the subnet for arp values and checks if there is no weird behaviour.

function IPv4toBin ($i){
    $b = $i -split '\.' | ForEach-Object {[System.Convert]::ToString($_,2).PadLeft(8,'0')}
    return $b -join ""
}

function Get-IPs($subnet)
{
    #Split IP and subnet
    $IP = ($Subnet -split "\/")[0]
    $SubnetBits = ($Subnet -split "\/")[1]
    $IPInBinary = IPv4toBin($IP)

    $HostBits = 32-$SubnetBits
    $NetworkIDInBinary = $IPInBinary.Substring(0, $SubnetBits)
    $HostIDInBinary = $IPInBinary.Substring($SubnetBits, $HostBits)
    $HostIDInBinary = $HostIDInBinary -replace "1","0"

    $imax = [convert]::ToInt32(("1" * $HostBits),2) -1

    $IPs = @()

    For ($i = 1 ; $i -le $imax ; $i++)
    {
        $NextHostIDInDecimal = ([convert]::ToInt32($HostIDInBinary,2) + $i)
        $NextHostIDInBinary = [convert]::ToString($NextHostIDInDecimal,2)
        $NoOfZerosToAdd = $HostIDInBinary.Length - $NextHostIDInBinary.Length
        $NextHostIDInBinary = ("0" * $NoOfZerosToAdd) + $NextHostIDInBinary
        $NextIPInBinary = $NetworkIDInBinary + $NextHostIDInBinary

        $IP = @()
        For ($x = 1 ; $x -le 4 ; $x++)
        {
            $StartCharNumber = ($x-1)*8
            $IPOctetInBinary = $NextIPInBinary.Substring($StartCharNumber,8)
            $IPOctetInDecimal = [convert]::ToInt32($IPOctetInBinary,2)
            $IP += $IPOctetInDecimal
        }
        $IP = $IP -join "."
        $IPs += $IP

    }
    return $IPs
}

function Check-Spoof($iplist)
{
    $gate_ip = ipconfig | Select-String "Default Gateway" | % { $_.ToString().Split(':')[1].Trim() }
    $gate_ip = $gate_ip[0]
    $gate_mac = arp -a $gate_ip | Select-String $gate_ip |  % { $_.ToString() -split "\s+" }
    $gate_mac = $gate_mac[2].ToUpper()

    echo ""
    echo "Gateway with ip: $gate_ip and mac: $gate_mac"
    echo ""

    for ($i = 0; $i -lt $iplist.Count; $i++)
    {
        if ($iplist[$i] -ne $gate_ip)
        {
            $ip = $iplist[$i]
            $arp_call = arp -a $ip | Select-String "$ip"
            if ($arp_call -ne $null)
            {
                  $call = $arp_call -split "\s+"
                  $mac = $call[2].ToUpper()
                  echo "Active IP: $ip with MAC: $mac"

                  if ($mac -eq $gate_mac)
                  {
                     echo ""
                     echo "You are being Spoofed a duplicate ARP value found"
                     return
                  }
            }
        }
    }
    echo ""
    echo "You are Safe, no duplicate ARP values found."
}

$subnet = "192.168.1.1/24"
$IPLIST = Get-IPs($subnet)
Check-Spoof($IPLIST)

I am fairly new to PowerShell, so I'm mostly concerned about the proper syntax.

split and spread to variables:

$IP, $SubnetBits = $Subnet -split '/'

your user-function invocation with parentheses is incorrect and works only for single parameter (basically it's interpreted as user-function (param1), the correct syntax is user-function param1 param2 param3 e.g. IPv4toBin $IP
to get a power of 2 use a bitwise left shift:

$imax = (1 -shl $HostBits) - 2

use $array = foreach ($i in 1..$imax) { ... } to generate arrays directly instead of overly verbose for loops and += operator, which recreates the entire array each time
ipconfig may list many networks with empty gateways so you need to account for that or use .NET method (C# source):

$gate_ip = [Net.NetworkInformation.NetworkInterface]::GetAllNetworkInterfaces() |
  where { $_.OperationalStatus -eq 'Up' -and $_.NetworkInterfaceType -ne 'Loopback' }|
  foreach { $_.GetIPProperties().GatewayAddresses } |
  select -expand Address -First 1 |
  select -expand IPAddressToString

Select-String $gate_ip will incorrectly select the header line with no ARP info in case the interface IP address matches the gateway IP e.g. 192.168.1.1 and 192.168.1.176
work with actual numbers instead of binary representation and use bitwise operators:

function Get-IPs($subnet) {
  $IP, $SubnetBits = $Subnet -split '/'
  [uint32]$imax = (1 -shl (32 - $SubnetBits)) - 2
  [uint32]$baseIP = 0
  foreach ($s in ($IP -split '\.')) {
    $baseIP = ($baseIP -shl 8) + $s
  }
  $baseIP = $baseIP -band -bnot ($imax + 1)
  foreach ($i in 1..$imax) {
    $IP = $baseIP + $i
    "$($IP -shr 24).$($IP -shr 16 -band 255).$($IP -shr 8 -band 255).$($IP -band 255)"
  }
}

I'm using implicit output here: a statement that consists only of a value (or an expression) is equal to Write-Output $value_or_expression and the entire collected output is returned from the function without the need for an explicit return


Measuring method execution time I want to easily measure method's execution time. Of course instead of doing something like this:

var timer = Stopwatch.StartNew();
DoSomething();
timer.Stop();
Console.WriteLine(timer.ElapsedMilliseconds)

I want to make it more elegant. What I've managed is something like this:

static class MethodMeasurer
{
    public static T Invoke<T>(Func<T> action, out Stopwatch stopwatch)
    {
        var timer = Stopwatch.StartNew();
        var res = action.Invoke();
        timer.Stop();
        stopwatch = timer;
        return res;
    }

    public static void Invoke(Action action, out Stopwatch stopwatch)
    {
        var timer = Stopwatch.StartNew();
        action.Invoke();
        timer.Stop();
        stopwatch = timer;
    }
}

And here is the usage:

class Program
{
    static void Main(string[] args)
    {
        MethodMeasurer.Invoke(DoSomething1, out var s1);
        Console.WriteLine("s1 executed time: " + s1.ElapsedMilliseconds);

        var doSomething2Measured = MethodMeasurer.Invoke(DoSomething2, out var s2);
        Console.WriteLine("s2 executed time: " + s2.ElapsedMilliseconds);
        Console.WriteLine("s2 returned: " + doSomething2Measured);

        var doSomething3Measured = MethodMeasurer.Invoke(() => DoSomething3(5), out var s3);
        Console.WriteLine("s3 executed time: " + s3.ElapsedMilliseconds);
        Console.WriteLine("s3 returned: " + doSomething3Measured);
    }

    static void DoSomething1()
    {
        Thread.Sleep(100);
    }

    static int DoSomething2()
    {
        Thread.Sleep(300);
        return 1;
    }

    static int DoSomething3(int i)
    {
        Thread.Sleep(300);
        return i;
    }
}

So what do you think about that? Would you suggest I use a different solution, or is mine okay?

Because you are interested in the time it took and not the stopwatch, you should return a TimeSpan. And since we no longer need stopwatch as a parameter, we can rename timer to stopwatch (as "timer" makes me think of the Timer class).

public static void Invoke(Action action, out TimeSpan timeSpan)
{
    var stopwatch = Stopwatch.StartNew();
    action.Invoke();
    stopwatch.Stop();
    timeSpan = stopwatch.Elapsed;
}


My web project security I started a web project. Now I was wondering if it is secure enough. I followed some tutorials on HTS, but I can't see all holes (if there are any). Can you check my pages and if you can find any hole in it (like XSS or SQL injections). Thanks for helping :)!

My URL is: **

And another security question: is my HTTPS certificate safe enough? (**) And my SMTP/SSH/... services?

You can now login using: user: demo, password: test123

User controller code:

<?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');

class User extends CI_Controller {

    public function create() {
        setHTTPS();
        if ($this->form_validation->run() == false) {
            $this->load->view('user/create');
        } else {
            $this->MUser->create();
            $this->load->view('user/email_sent');
        }
    }

    public function login() {
        setHTTPS();
        if ($this->form_validation->run() == false) {
            $this->load->view('user/login');
        } else {
            $this->MUser->createSession();
            $this->MUser->checkLogin();
            $this->load->view('user/dashboard');
        }
    }

    public function logout() {
        setHTTPS();
        if ($this->MUser->login()) {
            $this->MUser->logout();
            $this->load->view('user/logout');
        } else {
            $this->load->view('user/logout_failed');
        }
    }

    public function dashboard($username) {
        setHTTPS();
        $this->MUser->checkLogin();
        $this->load->view('/user/dashboard');
    }

    public function verify($userID, $code) {
        setHTTPS();
        $safeCode = urldecode($code);
        if ($this->MUser->checkVerifyCode($userID, $safeCode)) {
            $this->MUser->cleanVerifyCode($userID);
            $this->MUser->setActive($userID);
            mailSignupComplete($userID);
            $this->load->view('user/signup_complete');
        } else {
            $this->load->view('user/validation_failed');
        }
    }

    public function password($userID = null, $verifier = null) {
        setHTTPS();
        if ($userID !== null) {
            $safeCode = urldecode($verifier);
            if ($this->MUser->checkPasswordCode($userID, $safeCode)) {
                $this->MUser->updatePassword($userID);
                $this->load->view('user/password_succes');
            } else {
                $this->load->view('user/password_failed');
            }
        } else {
            if ($this->form_validation->run() == false) {
                $this->load->view('user/password');
            } else {
                $this->MUser->addRecoverPasswordEntry();
                $this->load->view('user/password_sent');
            }
        }
    }

}

/* End of file user.php */
/* Location: ./application/controllers/user.php */

Check your MySQL database. If you have another table called "websiteIsNotSecure" then you need to sanitize your database inputs. Also, it'd be easier to check security if we had some code to look at. You can't view PHP code, it's all parsed/executed on the server.


Menu system and Turtle I had to do a small application using a menu system and Turtle. I have finished it and worked perfect. Just to make sure, can you please check it if it has done properly?

#!/usr/bin/python

## draw_shape.py

import turtle as t

def how_many():
    while True:
        print "  How many of then do you want to draw?"
        print "  -Range is from 1 to 5-"
        global shape_no
        shape_no = raw_input('  Enter your choice: ')
        try:
            shape_no = int(shape_no)
            if (1 <= shape_no <= 5):
                print "Your number is ok"
                break
            else:
                print
                print "from 1 to 5 only"
        except:
            print "Only numbers allowed - Please try again"
    return True

#=========#=========#=========#=========#=========#=========#=========#
def start_point():
    t.penup()
    t.setpos(-240,0)
    t.pendown()

def draw_square(howmanyof):
    start_point()
    for a in range(howmanyof):
        for a in range(4):
            t.forward(80)
            t.left(90)
        t.penup()
        t.forward(100)
        t.pendown()

def draw_triangle(howmanyof):
    start_point()
    for a in range(howmanyof):
        for a in range(3):
            t.forward(80)
            t.left(120)
        t.penup()
        t.forward(100)
        t.pendown()

def draw_rectangle(howmanyof):
    start_point()
    for a in range(howmanyof):

        for a in range(2):
            t.forward(80)
            t.left(90)
            t.forward(40)
            t.left(90)
        t.penup()
        t.forward(100)
        t.pendown()

def draw_hexagon(howmanyof):
    start_point()
    for a in range(howmanyof):
        for a in range(6):
            t.forward(50)
            t.left(60)
        t.penup()
        t.forward(110)
        t.pendown()

def draw_octagon(howmanyof):
    start_point()
    for a in range(howmanyof):
        for a in range(8):
            t.forward(40)
            t.left(45)
        t.penup()
        t.forward(110)
        t.pendown()

def draw_circle(howmanyof):
    start_point()
    for a in range(howmanyof):
        t . circle(50)
        t.penup()
        t.forward(120)
        t.pendown()

#=========#=========#=========#=========#=========#=========#=========#
def main():
    while True:
        print
        print "  Draw a Shape"
        print "  ============"
        print
        print "  1 - Draw a square"
        print "  2 - Draw a triangle"
        print "  3 - Draw a rectangle"
        print "  4 - Draw a hexagon"
        print "  5 - Draw an octagon"
        print "  6 - Draw a circle"
        print
        print "  X - Exit"
        print

        choice = raw_input('  Enter your choice: ')

        if (choice == 'x') or (choice == 'X'):
            break
        elif choice == '1':
            how_many()
            draw_square(shape_no)
        elif choice == '2':
            how_many()
            draw_triangle(shape_no)
        elif choice == '3':
            how_many()
            draw_rectangle(shape_no)
        elif choice == '4':
            how_many()
            draw_hexagon(shape_no)
        elif choice == '5':
            how_many()
            draw_octagon(shape_no)
        elif choice == '6':
            how_many()
            draw_circle(shape_no)
        else:
            print
            print '  Try again'
            print

#=========#=========#=========#=========#=========#=========#=========#
if __name__ == "__main__":
    main()

#=========#=========#=========#=========#=========#=========#=========#

Why are you using a global? There seems to be no need for it, and in general it's something to avoid.
Why not factor out the common stuff? In your main loop you have a bunch of if cases which all start in the same way and then all call methods which are very similar. If you start by checking that it's a valid input you could pull out the calls to how_many() and start_point() and even the loop for a in range(howmanyof):.


error management when parsing multiple floats I am writing a handler to render a GIF of a Lorentz attractor. I need to parse floating point numbers from the some querystrings attached to the GIF path. If any of them are bad, I need to log an error.

func lorenzHandler(w http.ResponseWriter, r *http.Request) {
    var e error
    rho := atof(r.FormValue("r"), &e)
    sigma := atof(r.FormValue("s"), &e)
    b := atof(r.FormValue("b"), &e)
    if e != nil {
        log.Printf("bad parameters: r=%s; s=%s; b=%s;\n", r.FormValue("r"), r.FormValue("s"), r.FormValue("b"))
    } else {
        // draw lorenz GIF with parameters rho, sigma and b
        // but until then...
        log.Println("yay!", rho, sigma, b)
    }
}

func atof(s string, e *error) float32 {
    if *e == nil {
        var x float64
        x, *e = strconv.ParseFloat(s, 32)
        return float32(x)
    }
    return 0
}

Is passing around an error, then cleaning it up, correct?

I miss the reason you are working with a pointer of an error and passing it in different functions.

Why not doing in the simple way?

func lorenzHandler(w http.ResponseWriter, r *http.Request) {
    rho, e1 := strconv.ParseFloat(r.FormValue("r"), 32)
    sigma, e2 := strconv.ParseFloat(r.FormValue("s"), 32)
    b, e3 := strconv.ParseFloat(r.FormValue("b"), 32)
    if e1 != nil || e2 != nil || e3 != nil {
        log.Printf("bad parameters: r=%s; s=%s; b=%s;\n", r.FormValue("r"), r.FormValue("s"), r.FormValue("b"))
        w.WriteHeader(http.StatusBadRequest)
        return
    }

    // draw lorenz GIF with parameters rho, sigma and b
    // but until then...
    log.Println("yay!", rho, sigma, b)
}

If the problem is about performance, and you want to stop after the first error, without waiting all three executions, you can do

func lorenzHandler(w http.ResponseWriter, r *http.Request) {
    rho, err := strconv.ParseFloat(r.FormValue("r"), 32)
    if err != nil {
        log.Printf("bad parameters: r=%s\n", r.FormValue("r"))
        w.WriteHeader(http.StatusBadRequest)
        return
    }

    sigma, err := strconv.ParseFloat(r.FormValue("s"), 32)
    if err != nil {
        log.Printf("bad parameters: s=%s\n", r.FormValue("s"))
        w.WriteHeader(http.StatusBadRequest)
        return
    }

    b, err := strconv.ParseFloat(r.FormValue("b"), 32)
    if err != nil {
        log.Printf("bad parameters: b=%s\n", r.FormValue("b"))
        w.WriteHeader(http.StatusBadRequest)
        return
    }

    // draw lorenz GIF with parameters rho, sigma and b
    // but until then...
    log.Println("yay!", rho, sigma, b)
}


Justifying a string followed by a number I started practicing on HackerRank to see how my learning is going on with Java.
I wrote this code for problem which involved input having:

  String with maximum length of 10, followed by
  A 3 digit or lesser integer

Examples: "java 100", "Amazing 98"

Expected output:

  String should be left-justified with trailing white spaces till 15th character
  Leading digit of integer starts at 16th place with leading zeroes if it is less than 3 digits(padding) to make up for 18 characters

Examples: "java           100", "Amazing        098"

package HackerRank;

import java.util.ArrayList;
import java.util.Scanner;

public class IOFormatting {
    public ArrayList<String> getAllLines(){
        ArrayList<String> allInputs = new ArrayList<String>();
        Scanner getIp = new Scanner(System.in);
        while(getIp.hasNext()) {
            allInputs.add(getIp.nextLine());
        }
        getIp.close();
        return allInputs;
    }

    public ArrayList<Character> getTheInputAsArrayList(String input) {
        ArrayList<Character> inputLine = new ArrayList<Character>();
        for(int itr1=0;itr1<input.length();itr1++) {
            inputLine.add(input.charAt(itr1));
        }
        return inputLine;
    }

    public ArrayList<Character> formatTheArrayList(ArrayList<Character> inputLine){
        int itr2 = getBreakPoint(inputLine);
        for(int itr3=itr2+1;itr3<15;itr3++) {
            inputLine.add(itr3,' ');
        }
        return inputLine;
    }

    public int getBreakPoint(ArrayList<Character> inputLine){
        for(int itr2=0;itr2<=10;itr2++) {
            if(inputLine.get(itr2) == ' ') {
                return itr2;
            }
        }
        return 0;
    }

    public ArrayList<Character> doIntegerPadding(ArrayList<Character> inputLine){
        int padSize = 18 - inputLine.size();
        if(padSize>0) {
            for(int itr4=15;itr4<=14+padSize;itr4++) {
                inputLine.add(itr4,'0');
            }
        }
        return inputLine;
    }

    public String convertToFinalString(ArrayList<Character> inputLine) {
        String outputString = "";
        StringBuffer sb = new StringBuffer();
        for (Character s : inputLine) {
             sb.append(s);
        }
        outputString = sb.toString();
        return outputString;
    }

    public static void main(String args[]) {
        IOFormatting io = new IOFormatting();
        ArrayList<String> allInputs = io.getAllLines();
        System.out.println("================================");
        for(int mainItr=0;mainItr<allInputs.size();mainItr++) {
            String input = allInputs.get(mainItr);
            ArrayList<Character> inputLine = io.getTheInputAsArrayList(input);
            inputLine = io.formatTheArrayList(inputLine);
            inputLine = io.doIntegerPadding(inputLine);
            String outputString = io.convertToFinalString(inputLine);
            System.out.println(outputString);
        }
        System.out.println("================================");
    }
}

You are vastly overcomplicating the solution.  The Scanner is perfectly capable of reading a string and an integer separately.  Then, it is a simple matter of calling System.out.printf() with the appropriate width specifications:

%-15s for a string, left-justified, with trailing spaces
%03d for an integer, zero-padded

Also, Scanner is AutoCloseable, so you should use a try-with-resources block instead of calling .close() on it manually.

import java.util.Scanner;

public class IOFormatting {
    public static void main(String[] args) {
        try (Scanner input = new Scanner(System.in)) {
            while (input.hasNext()) {
                System.out.printf("%-15s%03d%n", input.next(), input.nextInt());
            }
        }
    }
}

If you really wanted to do the formatting the hard way by inserting spaces and zeroes, I suggest editing each line using StringBuilder.insert() — your ArrayList<Character> is a rather unnatural way to edit a string.