Java alphabeta search Is this code correct?

This code was adapted from this alphabeta pseudocode.  AlphaBeta search is often mentioned but getting the code 100% correct is tricky.

IGame is an interface with two methods:
getScore() - evaluates the current game state from the viewpoint of the AI
generateMoves() - creates possible moves from the current game state.

class Search
{
    Move bestMove;

    Move findBestMove(int ply, IGame g)
    {
        bestMove = null;
        alpha_beta(ply, g, Integer.MINIMUM, Integer.MAXIMUM);
        return bestMove;
    }

    int alpha_beta(int depth, IGame g, int alpha, int beta)
    {
        if (depth == 0 || g.isGameFinished()) return g.getScore();

        Move[] gameMove = g.generateMoves();

        for (int i = 0; i < gameMove.length; i++)
        {
            g.execute(gameMove[i]);
            int score = -alpha_beta(depth - 1, -beta, -alpha);
            g.undo(gameMove[i]);

            if (alpha < score)
            {
                alpha = score;
                bestMove = gameMove[i];
            }
            if (beta <= alpha) return alpha;
        }
        return alpha;
    }
}

My last AI course was too long time ago so just some generic notes and some modifications for easier testing.

Fields should be private. (Item 13 of Effective Java 2nd Edition: Minimize the accessibility of classes and members)
IGame could be Game.

  I don’t want my users knowing that I’m handing them an interface.
  I just want them to know that it’s a ShapeFactory.

Source: Clean Code by Robert C. Martin, Interfaces and Implementations, p24
Class names should be nouns, instead of verbs, like Search:

  Class and Interface Type Names

  Names of class types should be descriptive nouns or noun phrases, not overly long, in mixed
  case with the first letter of each word capitalized.

Source: The Java Language Specification, Java SE 7 Edition, 6.1 Declarations
What is ply? I guess maxDepth would be a more descriptive name.
Short variable names (like g) are not too readable. I suppose you have autocomplete, so using longer names does not mean more typing but it would help readers and maintainers a lot since they don't have to remember the purpose of each variable - the name would express the programmers intent. (Clean Code by Robert C. Martin, Avoid Mental Mapping, p25)

Make it testable

  Good design is testable, and design that isn't testable is bad.

(Source Michael C. Feathers: Working Effectively with Legacy Code)

I made some changes in the API (Game and Move classes to make it easier to test).

I've added a Move parameter to the generateMoves() method to make it stateless. It results easier mocking since the output only depends only on the input parameter.
I'm using move.isLeaf() instead of g.isGameFinished() (for the same reason). It could be game.isFinishingMove(Move move) too but the former was simpler.
I've created a new method for the Move class:

public String getName() { ... }

It will helper when we'll verificate the test results.
I've also changed the loop to a foreach. It also eliminated the duplicated gameMove[i] access.

So, the AlphaBetaSearcher class is the following now:

private Move bestMove;

public Move findBestMove(int maxDepth, Game game, Move root) {
    bestMove = null;
    alpha_beta(maxDepth, game, root,
        Integer.MIN_VALUE, Integer.MAX_VALUE);
    return bestMove;
}

private int alpha_beta(final int depth, final Game game,
        final Move move, int alpha, final int beta) {
    if (depth == 0 || move.isLeaf()) {
        return game.getScore();
    }

    final Move[] moves = game.generateMoves(move);
    for (final Move currentMove: moves) {
        game.execute(currentMove);
        final int score =
            -alpha_beta(depth - 1, game, currentMove, -beta, -alpha);
        game.undo(currentMove);

        if (alpha < score) {
            alpha = score;
            bestMove = currentMove;
        }
        if (beta <= alpha) {
            return alpha;
        }
    }
    return alpha;
}

Test data

For the test below I've used the leftmost part of the sample tree from Wikipedia.

Test framework

I'm using Mockito and JUnit here. A mocking framework makes possible to test the Alpha-beta implementation without the actual Game and Move classes. In isolation you can make sure that the pruning works well before you integrate it into the game. If it isn't completely bug-free it's easier to debug and modify in isolation.

JUnit tests are self-checking, you don't have to check the output and decide if it's OK or not. The assert* assertion methods throws an exception when the tested class does not return the excepted value.

Test code

import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.runners.MockitoJUnitRunner;

@RunWith(MockitoJUnitRunner.class)
public class AlphaBetaSearcherOrigTest {

    private final AlphaBetaSearcherOrig searcher =
        new AlphaBetaSearcherOrig();

    // Mockito creates a dummy, programmable Game instance for us
    @Mock
    private Game game;

    @Test
    public void testSubTree() {
        // we are creating the move tree with names and scores
        final Move root = createMockedMove("root");
        final Move moveA = createMockedMove("A");
        final Move moveB = createMockedMove("B");
        final Move moveAA = createLeafMove("AA", 5);
        final Move moveAB = createLeafMove("AB", 6);
        final Move moveBA = createLeafMove("BA", 7);
        final Move moveBB = createLeafMove("BB", 4);
        final Move moveBC = createLeafMove("BC", 5);

        // programming the dummy Game instance
        // calling generateMoves() with the root node will
        // return moveA and moveB
        when(game.generateMoves(root)).
            thenReturn(newMoveArray(moveA, moveB));
        when(game.generateMoves(moveA)).
            thenReturn(newMoveArray(moveAA, moveAB));
        when(game.generateMoves(moveB)).
            thenReturn(newMoveArray(moveBA, moveBB, moveBC));

        final Move bestMove = searcher.findBestMove(5, game, root);
        // checking the name of the best move
        assertEquals("BA", bestMove.getName());
    }

    private Move createMockedMove(String name) {
        // creating a dummy/programmable Move instance
        // whose getName() method will return name
        final Move mock = mock(Move.class);
        when(mock.getName()).thenReturn(name);
        return mock;
    }

    private Move createLeafMove(String name, int score) {
        final Move move = createMockedMove(name);
        when(move.isLeaf()).thenReturn(true);
        when(move.getScore()).thenReturn(score);
        return move;
    }

    private Move[] newMoveArray(Move... moves) {
        return moves;
    }
}

The algorithm currently return with BA. If I'm right it should be A but I'm really not sure.


Java exhaustive solution This is a take on Pebble Solitaire. The user inputs n number of games and after that inputs a strings of length 23 that contains a combinations of only 'o' (pebble) and '-' (empty space). If there are 2 adjacent pebbles and an empty space on either side, ie (oo- OR -oo), then you remove the middle pebble and you swap other two pieces with each other, ex 'oo-' will turn into '--o'.

My current approach is pretty much an exhaustive approach where it tries out every possible move and results the move set with the least number of pebbles left.

My program is working fine, in terms of output, but for some of my test cases it takes too long to find an answer (sometimes taking 18 seconds). I would like to know how I can improve the performance of my code without making it multithreaded.

package Pebble;

import java.util.Scanner;

public class PebbleSolitaire {

    public static void main(String[] args){

        Scanner input = new Scanner(System.in);
        int numOfGames = Integer.parseInt(input.nextLine());

        while (numOfGames > 0){

            char[] values = input.nextLine().toCharArray();
            long startTime = System.nanoTime();
            System.out.println(solve(values));
            System.out.println("Time to finish in ms: " + (System.nanoTime() - startTime) / 1000000);
            numOfGames--;
        }
        input.close();
    }

    private static int solve(char[] game){

        if(game != null && game.length == 0){
            return -1;
        }

        int result = 0;

        for (int i = 0; i < game.length; i++){
            if(game[i] == 'o'){
                result++;
            }
        }

        //print(game);

        for (int i = 0; i < game.length; i++ ){

            char[] temp = new char[game.length];
            copyArray(temp, game);

            if (i-2 >= 0 && temp[i] == '-' && temp[i-2] == 'o' && temp[i-1] == 'o'){//move pebble forwards
                temp[i-1] = temp[i-2] = '-';
                temp[i] = 'o';
                result = Math.min(result, solve(temp));
            }

            copyArray(temp, game);

            if(i+2 < temp.length && temp[i] == '-' && temp[i+1] == 'o' && temp[i+2] == 'o'){//move pebble backwards
                temp[i+1] = temp[i+2] = '-';
                temp[i] = 'o';
                result = Math.min(result, solve(temp));
            }
        }
        return result;
    }

    private static void copyArray(char[] copy, char[] og){
        for(int x = 0; x < copy.length; x++){
            copy[x] = og[x];
        }
    }
    private static void print(char[] c){
        for(char ch: c){
            System.out.print(ch);
        }
        System.out.println();
    }
}

My sample input and output:

2
-o----ooo----o----ooo--
6
Time to finish in ms: 0
oooooooooo-ooooooooooo-
4
Time to finish in ms: 18149

Don't copy the array

A lot of the time spent in your program is just making copies of the game array.  Instead of copying the array, just use the same one.  All you need to do is to undo the move you made after you are done recursing on it.  In other words:

Make the move
Recursively solve
Undo the move

Don't count the number of o's

Another time waster is the part where you count the number of o's on every recursive call.  Instead of doing that, just count the number once, at the very beginning.  Then when you recurse, pass in the new count.  The new count is always one less than the previous count because each move removes one o from the board.

Error check should only happen once

Similarly, this error checking:

    if(game != null && game.length == 0){
        return -1;
    }

only needs to be done once.  Having that code be in every recursive call is just slowing things down.

Memoization

Another thing that could be slowing your program down is the possibility that you are solving the same game states multiple times.  For example, from the starting position, making move A followed by move B might end up in game state S1.  But making move B followed by A might end up in the same state S1.  Right now your program would solve the S1 state twice.

What you could do is to create an array (or Map) to record the answers to any state you have already solved.  Since there are 23 spots, there are \$2^{23}\$ possible game states, or around 8 million.  You can number each game state based on the binary number you get from the board, where - is a 0 and o is a 1.  For example, --------------------o-o would translate to the binary number 00000000000000000000101, or 5.  That would be the "index" of your game state.

After solving for a game state, you record the result in your array or Map.  At the beginning of your function, you check the array/Map to see if the current position has already been solved, and just return the result if it has.

Also you can use bit tricks to compute the new game state index from the previous game state index, so you don't have to recompute it every time.  For example, if the index is index, and you are making a move starting at index i, then the new game state index would be index ^ (0x7 << (20-i)).  This is because you are flipping the board positions at the three bits starting at 20-i.

This memoization is probably going to help speed up your program more than the other tips, because it will cut entire branches off your search space instead of just speeding up your program by a flat percentage.

Sample rewrite

I used the ideas above to rewrite your program.  For the memoization part, I chose to use a HashSet to remember which game states were already visited.  I found that there was no need to actually remember the score for previously visited states (hence the Set and not a Map).

import java.util.HashSet;
import java.util.Scanner;

public class pebble {
    private static final int MAX_GAME_LENGTH = 31;

    public static void main(String[] args)
    {
        Scanner input = new Scanner(System.in);
        int numOfGames = Integer.parseInt(input.nextLine());

        while (numOfGames > 0) {
            char[] values = input.nextLine().toCharArray();

            if (values.length > MAX_GAME_LENGTH) {
                System.out.println("Skipping game: too large");
                continue;
            }
            long startTime = System.nanoTime();
            System.out.println(solve(values));
            System.out.println("Time to finish in ms: " +
                    (System.nanoTime() - startTime) / 1000000);
            numOfGames--;
        }
        input.close();
    }

    private static int solve(char[] game)
    {
        int              gameState = 0;
        int              score     = 0;
        HashSet<Integer> visited   = new HashSet<Integer>();

        // Compute initial game state and score.
        for (int i=0;i<game.length;i++) {
            gameState <<= 1;
            if (game[i] == 'o') {
                gameState |= 1;
                score++;
            }
        }

        return solveHelper(gameState, score, visited, game.length);
    }

    private static int solveHelper(int gameState, int score,
            HashSet<Integer> visited, int gameLength)
    {
        int bestScore = score;

        visited.add(gameState);
        for (int i = gameLength-3; i >= 0; i--) {
            int trio = (gameState >> i) & 0x7;
            if (trio == 0x6 || trio == 0x3) {
                // 0x6: oo- -> --o
                // 0x3: -oo -> o--
                int mask         = ~(0x7 << i);
                int newGameState = (gameState & mask) | ((trio ^ 0x7) << i);
                if (!visited.contains(newGameState)) {
                    int ret = solveHelper(newGameState, score-1, visited,
                                            gameLength);
                    if (ret < bestScore)
                        bestScore = ret;
                }
            }
        }
        return bestScore;
    }
}

Optimizations helped a lot

I ran three programs to solve the difficult board.  The first was the original program.  The second was a variant of my program with the visited set removed to test how much of a difference the memoization made.  The third was my sample rewrite program unmodified.  Here are the results:

Original program : 41000 ms
JS1 (no memo)    :  1460 ms (28x  faster)
JS1 (with memo)  :     3 ms (480x faster than no memo)


Window-size-dependent navigation menu with animation effects Other people will be looking at this jquery code and i'm not an expert with jQuery so im asking. How can i make this jQuery code shorter, more efficient, and easier to read?

/*
HEADER NAVIGATION
*/
/* navigation tabs click highlight */
$(".header-main-tabs").click(function() {
$(".header-main-tabs").removeClass("header-tab-selected");
$(this).addClass("header-tab-selected");
});
/* drop menu hide and show for desktop */
$(".header-main-tabs").hover(function() {
$(this).children(".header-drop-menu").toggleClass("show-drop-menu-hover");
});

/* search input hide and show when search icon is pressed */
$("#search-icon-container span").click(function() {
$(this).toggleClass("fa-times");
$("#search-input-container").fadeToggle("fast");
});

/* mobile navigation */
/* show mobile tabs when toggle nav mobile button is clicked and when browser width is      over 990px */
/* toggle mobile navigation when nav button is clicked */
$("#toggle-mobile-nav").click(function() {
$("#nav-tabs-list").slideToggle();
$("#toggle-mobile-nav").toggleClass("toggle-mobile-nav-clicked");
});

var browserWidth = $(window).width();

$(window).resize(function() {
browserWidth = $(window).width();
if (browserWidth > 990) {
    $("#nav-tabs-list").show();
}
 });

$('#nav-tabs-list li').click(function() {
if (browserWidth <= 990) {
    $('#nav-tabs-list').slideUp();
}
});

/* always show drop menu when on mobile version (when browser width is below 960px) */
$(document).ready(function() {
fadeMobile();
});

$(window).resize(function() {
  fadeMobile();
});

function fadeMobile() {
  browserWidth = $(window).width();
  if (browserWidth < 990) {
$("#nav-tabs-list").hide();
    $('.header-drop-menu').show();
    $("#toggle-mobile-nav").removeClass("toggle-mobile-nav-clicked");
      }
    }

Easier to Read

Make sure you use the same standard everywhere, indent nicely and be consistent. Then the code already looks a lot better:

/*
HEADER NAVIGATION
*/

/* navigation tabs click highlight */
$(".header-main-tabs").click(function() {
    $(".header-main-tabs").removeClass("header-tab-selected");
    $(this).addClass("header-tab-selected");
});

/* drop menu hide and show for desktop */
$(".header-main-tabs").hover(function() {
    $(this).children(".header-drop-menu").toggleClass("show-drop-menu-hover");
});

/* search input hide and show when search icon is pressed */
$("#search-icon-container span").click(function() {
    $(this).toggleClass("fa-times");
    $("#search-input-container").fadeToggle("fast");
});

/* mobile navigation */
/* show mobile tabs when toggle nav mobile button is clicked and when browser width is      over 990px */
/* toggle mobile navigation when nav button is clicked */
$("#toggle-mobile-nav").click(function() {
    $("#nav-tabs-list").slideToggle();
    $("#toggle-mobile-nav").toggleClass("toggle-mobile-nav-clicked");
});

var browserWidth = $(window).width();

$(window).resize(function() {
    browserWidth = $(window).width();
    if (browserWidth > 990) {
        $("#nav-tabs-list").show();
    }
});

$('#nav-tabs-list li').click(function() {
    if (browserWidth <= 990) {
        $('#nav-tabs-list').slideUp();
    }
});

/* always show drop menu when on mobile version (when browser width is below 960px) */
$(document).ready(function() {
    fadeMobile();
});

$(window).resize(function() {
    fadeMobile();
});

function fadeMobile() {
    browserWidth = $(window).width();
    if (browserWidth < 990) {
        $("#nav-tabs-list").hide();
        $('.header-drop-menu').show();
        $("#toggle-mobile-nav").removeClass("toggle-mobile-nav-clicked");
    }
}

Shorter

Don't use $('...') twice for the same selector and don't use it in an event. Instead, save the element in a variable and use that. It's both faster and easier to read
Group events on the same element. The first 2 events are both placed on $('.header-main-tabs'), group them together
Put all code in the $(document).ready() callback, this will avoid "DOM not ready" errors
Use CSS to hide/show elements, don't use jQuery for that
Combine the same events, you now have 2 $(window).resize() events
Don't show on each resize, only show if it was hidden before
Don't use global variables to store data

/*
HEADER NAVIGATION
*/

$(document).ready(function () {

    var $mainTabs = $(".header-main-tabs");
    var $searchInputContainer = $("#search-input-container");

    $mainTabs
        .click(function() { // navigation tabs click highlight
            $mainTabs.removeClass("header-tab-selected");

            $(this).addClass("header-tab-selected");
        })
        .hover(function() { // drop menu hide and show for desktop
            $(this).children(".header-drop-menu").toggleClass("show-drop-menu-hover");
        });

    /* search input hide and show when search icon is pressed */
    $("#search-icon-container span").click(function() {
        $(this).toggleClass("fa-times");

        $searchInputContainer.fadeToggle("fast");
    });

    /* mobile navigation */
    /* show mobile tabs when toggle nav mobile button is clicked and when browser width is      over 990px */
    /* toggle mobile navigation when nav button is clicked */
    var $navTabsList = $("#nav-tabs-list");
    var $toggleMobileNav = $("#toggle-mobile-nav");
    var $headerDropMenu = $('.header-drop-menu');

    $toggleMobileNav.click(function() {
        $navTabsList.slideToggle();

        $toggleMobileNav.toggleClass("toggle-mobile-nav-clicked");
    });

    $(window).resize(function() {
        browserWidth = $(window).width();

        if (browserWidth > 990 && $navTabsList.is(':hidden')) {
            $navTabsList.show();
        }

        fadeMobile();
    });

    $('li', $navTabsList).click(function() {
        if ($(window).width() <= 990) {
            $navTabsList.slideUp();
        }
    });

    /* always show drop menu when on mobile version (when browser width is below 960px) */
    fadeMobile();

    function fadeMobile(browserWidth) {
        if (browserWidth < 990) {
            if ($navTabsList.is(':visible')) {
                $navTabsList.hide();
            }

            if ($headerDropMenu.is(':hidden')) {
                $headerDropMenu.show();
            }

            if ($toggleMobileNav.hasClass('toggle-mobile-nav-clicked')) {
                $toggleMobileNav.removeClass("toggle-mobile-nav-clicked");
            }
        }
    }

});


Email validation using JavaScript I have a function where I validate emails via a regex. I was wondering if this is the best regex to use or if there's a better way of doing it.

Also is there a difference between this method and just setting the type="email" in the HTML form?

<form name="form" action="process_registration.jsp" onSubmit="return validateEmail()" method="POST">
        <script>
        function validateEmail(){
            var email = document.forms["form"]["Email"].value;
            var regex = /^([0-9a-zA-Z]([-_\\.]*[0-9a-zA-Z]+)*)@([0-9a-zA-Z]([-_\\.]*[0-9a-zA-Z]+)*)[\\.]([a-zA-Z]{2,9})$/;

            if(!regex.test(email)){
                alert("Enter a valid email");
                return false;
                }
        }

I disagree with rolfl's assertion that, "There is no practical way to validate an e-mail address by regex alone."

He is correct—as illustrated by the somewhat infamous SO answer he linked to—that it's impractical to validate any RFC-5322-compliant email addresses because, to quote the HTML5 spec:

  RFC-5322 … defines a syntax for e-mail addresses that is simultaneously too strict (before the '@' character), too vague (after the '@' character), and too lax (allowing comments, whitespace characters, and quoted strings in manners unfamiliar to most users) to be of practical use here.

...not to mention that there are now literally thousands of TLDs.

But it's absolutely practical to come up with a very good compromise, which is exactly what the writers of the HTML5 spec did. To that end, the spec provides this PCRE:

/^[a-zA-Z0-9.!#$%&'*+\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$/

This regular expression will yield false positives (rejections) for technically valid email addresses, including ones with spaces and comments (yes, there is a syntax for putting comments in an email address), but practically all email addresses in use in the real world will pass.

As rolfl suggests, the only perfect way to validate an email address is to send an email to it and see if it's received—not all syntactically valid email addresses actually exist, after all—but that doesn't mean we can't use a pretty good validation to give the user useful feedback in the client, and for that we can look to the very smart people who did the work for us when they wrote the HTML5 spec.

As to your second question:

  Also is there a difference between this method and just setting the type="email" in the HTML form?

Sure. Using type="email" makes the browser do the validation for you. That's obvious, I know, but it has a number of implications, some good and some not-necessarily-good. Among them:

The browser might not do it. It might not support automatic validations, or the user might have disabled it. This limitation could be overcome with a polyfill, however.
In browsers that support it, it just works, and you don't have to write any code, which is pretty nice.
Currently there's no widely-supported way to customize the way browsers show HTML5 validation errors, so you can't control how it'll look to the user.

I'm sure there are others, but you get the idea.

My personal recommendation is that unless you have a good reason not to, use type="email" along with a polyfill (if you have users with older browsers). Very smart people wrote the HTML5 spec, and very smart people wrote the browsers that implement it. Unless you know something that they don't, use the solutions they've already built.


Extracting data from a Word document is too slow I have a 100page long .docx format document. I'm using a macro written in VBS to extract some information and then just generate a table from them.  I iterate through the paragraphs and store the found strings in 3 separate arrays.

However, the loop is unreasonably slow. It takes 3 min to complete on a relatively fast computer. Can you take a look at it and tell me what causes this slowness?

'TODO : Add checks, exception handling, dynamic user options, probes, style checks, and fix slowness.

Sub genTable()

Dim objDoc

'''''''''''''''''''''Modify these''''''''''''''''
Dim ColumnName1, ColumnName2, ColumnName3, magicString
ColumnName1 = "foo1"
ColumnName2 = "foo2"
ColumnName3 = "foo3"
magicString = "ASD321: " ' we search for this string

Set objDoc = ActiveDocument ' Because we run inside of word as macro
''''''''''''''''''''''''''''''''''''''''''''''''''

Const MAX = 200 ' using fixed sized arrays, mod this if you'll have more than 200 entries.
Dim vulnerabilityArr(MAX) ' initializing the arrays
Dim severityArr(MAX)
Dim paragraphArr(MAX)

Dim counter ' will count the processed entries in this
counter = 0

Dim currParagraph ' will be set in loop
Dim tmpArray() As String ' for splitting
    For pIndex = 1 To objDoc.Paragraphs.Count ' THIS LOOP IS SLOW
        currParagraph = objDoc.Paragraphs(pIndex)
        currParagraph = Left(currParagraph, Len(currParagraph) - 1) 'remove junk character
        If InStr(1, currParagraph, magicString) > 0 Then ' assuming this string is always present and the other two target data is near it
            tmpArray = Split(currParagraph) ' extract level
            currParagraph = objDoc.Paragraphs(pIndex - 1) 'assuming the previous paragraph is the vuln. name

            'Storing the 3 extracted data
            vulnerabilityArr(counter) = currParagraph
            severityArr(counter) = tmpArray(1)
            paragraphArr(counter) = objDoc.Paragraphs(pIndex - 1).Range.ListFormat.ListString ' for some weird reason I cant use currParagraph here

            counter = counter + 1 ' adjusting index
        End If
    Next pIndex

objDoc.Tables.Add objDoc.Paragraphs(objDoc.Paragraphs.Count).Range, counter, 3, True, True
Set objTable = objDoc.Tables(objDoc.Tables.Count) 'select last table

objTable.Cell(1, 1).Range.Text = ColumnName1
objTable.Cell(1, 2).Range.Text = ColumnName2
objTable.Cell(1, 3).Range.Text = ColumnName3

For RowIndex = 0 To counter
    objTable.Cell(RowIndex, 1).Range.Text = paragraphArr(RowIndex)
    objTable.Cell(RowIndex, 2).Range.Text = vulnerabilityArr(RowIndex)
    objTable.Cell(RowIndex, 3).Range.Text = severityArr(RowIndex)
    Next
    RowIndexy = RowIndex + 1

objTable.AutoFormat (25)

End Sub

Maybe I'm deeply wrong, but please test:

-- Almost all your variables are of type Variant, even the ones which could be Long, like counter. Define each variable with the correct type (it will be faster), in the form:

  Option Explicit
  Dim counter as Long
  Dim ColumnName1 as String, ColumnName2 as String

-- You dont really use pIndex. Try:

Dim currParagraph As Paragraph               ' will be set in loop
For Each currParagraph In objDoc.Paragraphs  ' THIS LOOP IS SLOW

-- I dont see why you make a copy of the paragraph only to 'remove junk character ?
for some weird reason I cant use currParagraph here: I have always heard that this information is with this junk character stick

All will:

'TODO : Add checks, exception handling, dynamic user options, probes, style checks, and fix slowness.
Option Explicit
Sub genTable()

Dim objDoc

'''''''''''''''''''''Modify these''''''''''''''''
Dim ColumnName1 As String, ColumnName2 As String, ColumnName3 As String, magicString As String
ColumnName1 = "foo1"
ColumnName2 = "foo2"
ColumnName3 = "foo3"
magicString = "ASD321: " ' we search for this string

Set objDoc = ActiveDocument ' Because we run inside of word as macro
''''''''''''''''''''''''''''''''''''''''''''''''''

Const MAX = 200 ' using fixed sized arrays, mod this if you'll have more than 200 entries.
Dim vulnerabilityArr(MAX) As Paragraph ' initializing the arrays
Dim severityArr(MAX) As String
Dim paragraphArr(MAX) As String

Dim counter As Long ' will count the processed entries in this
counter = 0

Dim tmpArray() As String ' for splitting
Dim currParagraph As Paragraph, prevParagraph As Paragraph ' will be set in loop
For Each currParagraph In objDoc.Paragraphs  ' THIS LOOP IS SLOW
                                                  ' currParagraph = Left(currParagraph, Len(currParagraph) - 1) 'remove junk character
    If InStr(currParagraph, magicString) > 0 Then ' assuming this string is always present and the other two target data is near it
        tmpArray = Split(currParagraph.Range.Text)             ' extract level
        Set prevParagraph = currParagraph.Previous    ' assuming the previous paragraph is the vuln. name

            'Storing the 3 extracted data
        Set vulnerabilityArr(counter) = prevParagraph
        severityArr(counter) = tmpArray(1)
        paragraphArr(counter) = prevParagraph.Range.ListFormat.ListString ' for some weird reason I cant use currParagraph here

        counter = counter + 1 ' adjusting index
    End If
Next currParagraph

Dim objTable As Table
Set objTable = objDoc.Tables.Add(objDoc.Paragraphs(objDoc.Paragraphs.Count).Range, counter + 1, 3, True, True)
                                 'Set objTable = objDoc.Tables(objDoc.Tables.Count) 'select last table

objTable.Cell(1, 1).Range.Text = ColumnName1
objTable.Cell(1, 2).Range.Text = ColumnName2
objTable.Cell(1, 3).Range.Text = ColumnName3

Dim RowIndex As Long, RowIndexy As Long

For RowIndex = 0 To counter - 1
    objTable.Cell(RowIndex + 2, 1).Range.Text = paragraphArr(RowIndex)
    objTable.Cell(RowIndex + 2, 2).Range.Text = vulnerabilityArr(RowIndex).Range.Text
    objTable.Cell(RowIndex + 2, 3).Range.Text = severityArr(RowIndex)
Next
RowIndexy = RowIndex + 1

objTable.AutoFormat (25)

End Sub


Turning JSON objects into custom NSObjects Was advised to post this question here after posting to Stack Overflow. I'm pre-populating a UITableView app with data using a local JSON file. The JSON itself is very simple:

[
    {"Name":"Tennis Ball Launchers",
    "Type":"Forces"},

    {"Name":"Magnetic Hot Plates",
    "Type":"Magnetism"},

    {"Name":"Giant Lever",
    "Type":"Forces"},

    {"Name":"Music Wall",
    "Type":"Sound"},

    {"Name":"Water Bearing",
    "Type":"Forces"},

    {"Name":"Chaos Pendulums",
    "Type":"Chaos"}
]

The JSON objects will have more properties further down the line, this is just for starters (there will be around 200 entries by the end, with more properties). I don't imagine the structure getting any more complex, however. The user will not be adding or deleting entries in the app - the JSON is read only. I want to map these JSON objects into an array of custom NSObjects. Here is the code I am currently using in the initialisation of my data model:

    // JSON reading into array
    NSString *filePath = [[NSBundle mainBundle] pathForResource:@"test" ofType:@"json"];

    NSError *error;

    NSString *fileContents = [NSString stringWithContentsOfFile:filePath encoding:NSUTF8StringEncoding error:&error];

    if(error) {
        NSLog(@"Error reading file: %@", error.localizedDescription);
    }

    // Get JSON objects into initial array
    NSArray *rawExhibits = (NSArray *)[NSJSONSerialization JSONObjectWithData:[fileContents dataUsingEncoding:NSUTF8StringEncoding] options:0 error:NULL];

    NSMutableArray *myItems = [[NSMutableArray alloc] init];

    for (NSDictionary *object in rawExhibits) {
        NSString *objectName = object[@"Name"];
        NSString *objectType = object[@"Type"];

        // Create new exhibit object with data procured from JSON object
        W5BExhibit *exhibit = [[W5BExhibit alloc] initWithExhibitName:objectName exhibitType:objectType];

        [myItems addObject:exhibit];
    }

    self.allExhibits = [[NSArray alloc] initWithArray:myItems];

The table view can then refer to the allExhibits variable which is a property of a singleton. I'm very new at this so I'm canvassing for opinions.

I was also advised to consider using keyed archiving to persist the data instead of parsing the JSON each time the app is opened up from new.

The correct way to check for success or failure of Cocoa (Touch) methods is
documented in
"Handling Error Objects Returned From Methods" in the "Error Handling Programming Guide":

  Important: Success or failure is indicated by the return value of the
  method. Although Cocoa methods that indirectly return error objects in
  the Cocoa error domain are guaranteed to return such objects if the
  method indicates failure by directly returning nil or NO, you should
  always check that the return value is nil or NO before attempting to
  do anything with the NSError object.

Which means that the result of

NSString *fileContents = [NSString stringWithContentsOfFile:filePath encoding:NSUTF8StringEncoding error:&error];

should be checked with

if (fileContents == nil) { // Not: if (error) { ...
    NSLog(@"Error reading file: %@", error.localizedDescription);
}

And you should also use the error parameter and check the result of

NSArray *rawExhibits = (NSArray *)[NSJSONSerialization JSONObjectWithData:[fileContents dataUsingEncoding:NSUTF8StringEncoding] options:0 error:NULL];

The explicit cast to (NSArray *) is not necessary because the method
returns id and that can be assigned to any Objective-C object pointer.

Actually your method to read the JSON data is too complicated: The file
contents is first read into a string (using some encoding) and then converted to back to data (using the same encoding).
This can be simplified by reading the file into data in the first step:

NSData *jsonData = [NSData dataWithContentsOfFile:filePath];

You also have to decide what to do if reading the JSON data fails.
Just logging the error is not sufficient because then the program will
crash later.

The parameter naming in your init method

-initWithExhibitName:exhibitType:

seems a bit too verbose to me. It is clear from the class name W5BExhibit
that the object is some "exhibit", so I would shorten that too

-initWithName:type:

Adding new properties to your object class and the JSON file currently
requires a new init method with more arguments.
This can be greatly simplified using "Key-Value Coding" if you use the exact property names of your custom class as keys in the dictionary.

So assuming that W5BExhibit is defined as

@interface W5BExhibit : NSObject
@property(copy, nonatomic) NSString *name;
@property(copy, nonatomic) NSString *type;
@end

the JSON data should look like

[
  {"name":"Tennis Ball Launchers",
  "type":"Forces"},

  {"name":"Magnetic Hot Plates",
  "type":"Magnetism"},

  ...
]

Then you can create an object from a dictionary with

-(instancetype)initWithDictionary:(NSDictionary *)dict
{
    self = [super init];
    if (self) {
        [self setValuesForKeysWithDictionary:dict];
    }
    return self;
}

self.allExhibits = [[NSArray alloc] initWithArray:myItems];

can be simplified to

self.allExhibits = [myItems copy];

The complete code then looks like this (where I have renamed some variables,
this might be a matter of personal taste):

// Read JSON data into array
NSError *error;
NSString *jsonPath = [[NSBundle mainBundle] pathForResource:@"test" ofType:@"json"];
NSData *jsonData = [NSData dataWithContentsOfFile:jsonPath];
if (jsonData == nil) {
    // handle error ...
}
NSArray *jsonArray = [NSJSONSerialization JSONObjectWithData:jsonData options:0 error:&error];
if (jsonArray == nil) {
    // handle error ...
}

// Create custom objects from JSON array
NSMutableArray *items = [[NSMutableArray alloc] init];
for (NSDictionary *dict in jsonArray) {
    W5BExhibit *exhibit = [[W5BExhibit alloc] initWithDictionary:dict];
    [items addObject:exhibit];
}
self.exhibits = [items copy];


jQuery animate opacity, fade, & slide up page load effect This is the first time I've actually made my own jQuery script from scratch, because I couldn't find exactly what I was looking for.  My code works how I want it to, although I don't know if I did it correctly.  Just something about it is screaming to me, and although it works, I didn't do it properly.  So I figured I would share what I have and see what you had to offer as a better way to accomplish this.

$(window).load(function() {
    $('.overlay').animate({
        opacity: 1,
        height: '0'
    }, 1000, function(){$(this).remove()});

    $("#frame").delay(300).animate({
        opacity: 1
    }, 300)
});

I have ran it though a JavaScript compressor which removed the ;s, and then through a "beautifier" for readability, but it still seems to work.  I'm not even too sure how important those are in JavaScript.  I'm a CSS guy and I know in CSS the last ; is not required, so figured I'd leave them out since it's working.

I set up an example of this in action here.

If anyone has any suggestions on my code as this being my first real attempt at jQuery, I would love some feedback.

Interesting concept, and I do see some improvements that could be made.

First off though, it's true that the semicolons aren't necessary in JavaScript. It's a matter of preference really. If you think the people who will read your code would prefer semicolons, then use them where necessary. I suggets you try JSLint in the future!

I think that your animation is somewhat strange, but I'm not here to argue that! (The change in height of .overlay is what I'm talking about) I think that combination of animations is fine, and could be left as is.
Perhaps the one thing you could do: add some space to improve readability. Packing that last closure onto one line is just restrict breathing room, and it would be easier to quickly read it if it was spread out.
Your second animation could simply be:

$('#frame').delay(300).fadeIn(300)

You weren't the first one who thought about fading ;)

So my ideal function would be as follows:

$(window).load(function () {
    $('.overlay').animate({
        opacity: 1,
        height: 0
    }, 1000, function () {
        $(this).remove()
    });

    $("#frame").delay(300).fadeIn(300)
});


Converts a user inputed string into a Integer Just a little exercise to make me more affluent with strings and error handling, any improvements welcome!

#include <stdio.h>
#include <string.h>
#include <ctype.h>
#include <stdbool.h>
#include <math.h>

#define MAXSIZE 1024

void retreiveStringInput(char *str, size_t buffersize)
{
    for (;;) {
        if (fgets(str, buffersize, stdin) != NULL) {
            str[strcspn(str, "\n")] = 0;
            return;
        }
        else
            printf("Touble allocating memory, Please try again...\n");
    }
}

bool strHasAllDigits(char *str)
{
    size_t strsize = strlen(str);
    if (str[0] != 0) {
        for (size_t strIndex = 0; strIndex < strsize; ++strIndex) {
            if (isdigit(str[strIndex]))
                continue;
            else
                return false;
        }
        return true;
    }
    else {
        printf("String is empty...\n");
        return false;
    }
}

int strToInt(char str[])
{
    size_t len = strlen(str);
    size_t power = len - 1;
    int convertedString = 0;
    for (size_t strIndex = 0; strIndex < len; ++strIndex, --power) {
        convertedString += ((int)(str[strIndex] - '0') * pow(10,power));
    }
    return convertedString;
}

int main()
{
    char number[MAXSIZE];
    int intNumber = 0;
    for (;;) {
        printf("ENTER A WHOLE NUMBER: ");
        retreiveStringInput(number, MAXSIZE);
        if (strHasAllDigits(number))
            intNumber = strToInt(number);
        else {
            printf("Number must be a whole number containing no characters, please try again...\n\n");
            continue;
        }
        printf("Your number is: %d\n", intNumber);
        break;
    }

    printf("Press any key to continue...");
    getchar();
}

Nitpicks

void retreiveStringInput(char *str, size_t buffersize)

Misspelled.

void retrieveStringInput(char *str, size_t buffersize)

Not a critical mistake, since you used it consistently.  Don't forget to change other uses when you fix it.

            printf("Touble allocating memory, Please try again...\n");

This suggests that there is trouble allocating memory, but this code doesn't allocate memory.  It reads from input.

It also has a typo in "Trouble".

Keeping it simple

    size_t strsize = strlen(str);
    if (str[0] != 0) {
        for (size_t strIndex = 0; strIndex < strsize; ++strIndex) {
            if (isdigit(str[strIndex]))
                continue;
            else
                return false;
        }
        return true;
    }
    else {
        printf("String is empty...\n");
        return false;
    }

Consider

    if (*str == '\0') {
        printf("String is empty...\n");
        return false;
    }

    for (; *str != '\0'; ++str) {
        if (!isdigit(*str)) {
            return false;
        }
    }

    return true;

I find it easier to use positive if conditions with else statements.  So the if is the positive case and the else is the negative case.  In this situation, we don't need an else then, as we return in the if.

There is no point in a continue when there is no statement to skip.  Just let it go.  It will continue automatically.  You don't need an else case.

You only used the length as part of the condition check in the loop.  But we don't need it.  We can just check for the end of the string directly.  We know that the end of string marker is '\0', so we can just check for that.

It's true that '\0' is 0, but it is more readable to write it out in my opinion.

I changed str[0] to *str because I knew that I was going to be saying *str in the for loop anyway.  Rather than use two different things that return the same value, I picked the one that worked in both cases.  It would look kind of odd to repeatedly do str[0] and act as if the values would be different.  That's more understandable with *str.

I don't like the single statement form of control structures.  I always use the block form as being more robust against editing mistakes and a little easier to read.

Avoid unnecessary floating point operations

    size_t power = len - 1;
    int convertedString = 0;
    for (size_t strIndex = 0; strIndex < len; ++strIndex, --power) {
        convertedString += ((int)(str[strIndex] - '0') * pow(10,power));
    }

Consider

    int convertedString = 0;
    for (size_t strIndex = 0; strIndex < len; ++strIndex) {
        convertedString *= 10;
        convertedString += (int)(str[strIndex] - '0');
    }

We don't need power anymore.

We don't convert to and from a floating point type.  No danger of precision losses.  Everything is an integer type.  We probably don't even need the explicit cast to int.  I haven't run the code, so I left it.

Simplify

        if (strHasAllDigits(number))
            intNumber = strToInt(number);
        else {
            printf("Number must be a whole number containing no characters, please try again...\n\n");
            continue;
        }
        printf("Your number is: %d\n", intNumber);
        break;

Consider

        if (strHasAllDigits(number)) {
            int intNumber = strToInt(number);
            printf("Your number is: %d\n", intNumber);
            break;
        }

        printf("Number must be a whole number containing no characters, please try again...\n\n");

Now we don't have to declare intNumber outside the loop.  And we don't have to continue to skip the break.


Generating rooms in a map I'm working on a game in JavaScript and I'm trying to generate rooms in a map.  For this example, the map is 32 by 24 (each tile is 32 by 32 pixels).

A room is made up of a collection of tiles.  A room is always in a square or rectangle shape.  For example, 16,16,8,8 would make a room that draws from the top left corner at 16 by 16 and the bottom right corner at 24 by 24.

How the algorithm works:

pick a random x,y, width and height for the room
check if these points collide with another room
if it fails repeat points 1 and 2 (it will only repeat problems 1 and 2 1000 times before finally giving up and deciding it can't fit any more rooms in)

This is done by this code here:

    //make sure a room doesn't clash here
    while (maxTrys > 0) {
        var x = randomValue(2, tileX - MAXROOMSIZE); //starting top left corner tile's x position
        var y = randomValue(2, tileY - MAXROOMSIZE);//starting top left corner tile's y position

        var width = randomValue(MINROOMSIZE, MAXROOMSIZE); //width of room in tiles sqaures e.g 3 = 96 pixels
        var height = randomValue(MINROOMSIZE, MAXROOMSIZE);//height of room in tiles e.g 3 = 96 pixels

       if (locationIsFine(x, y, width, height) == true) { //if we've found a location we're happy with
           roomStore.push(createRoom(i, x, y, width, height));
            break;
        }
        maxTrys--;
    }

How it checks if a point collides with another room:

generate a room with the randomly created x,y,width and height cordinates
check if any point in this "temp room" collides with any point of any other room
if it does we know there is a collision

The code for this is the following:

var locationIsFine = function(x, y, width, height) {
    //turn the cordinates into a fake room
    var tempTiles = new Array();

    for (var i = 0; i < width; i++) {
        for (var j = 0; j < height; j++) {
            tempTiles.push(new tile(tileset,x+ j,y + i,0,null,ScreenManager));
        }
    }

    //make sure room wont hit any other rooms, we do this by checking if edges of a room collide with an exisiting room
    for (var i = 0; i < roomStore.length; i++) {
        for (var j = 0; j < tempTiles.length; j++) {
            if (roomStore[i].intersects(tempTiles[j].getX(), tempTiles[j].getY()) == true) {
                return false;
            }
        }
    }
    return true;
}

The intersects method looks like this:

this.intersects = function (x,y) {

    /* find the biggest and smallest points in the room*/
    for (var i = 0; i < tiles.length; i++) {
        if (tiles[i].getX() == x && tiles[i].getY() == y) {
            return true;
        }
    }
    return false;
}

My problem with this is that it is really really slow.  After running it a few times, it can take 5-8 seconds to generate the rooms and normally it gives up after the 5th room.  I am 100% sure the bottleneck is coming from this code as if I set maxTrys to a smaller number, the program runs quicker.

In your locationIsFine it seems you check for each room in your roomStore if any of its tiles intersect with a tile inside your new/fake room.

It would probably be worth wile to add some basic checks which (the larger your roomStore will grow, the more it) will reduce the number of checks needed.

This is actually a rectangle intersection problem.
Between rectangles, if one isn't completly to the left, right, above or below the other, they will intersect; so checking on that is the fastest way and will save you the check on each and every tile against each and every tile of the other room. This will save you from creating the tilesets for the fake room.

So a start to enhance the algorithm would be this.

var locationIsFine = function(x, y, width, height) {
    //make sure room wont hit any other rooms, we do this by checking if edges of a room collide with an exisiting room
    for (var i = 0; i < roomStore.length; i++) {
        // roomStore[i] completly left or completly right of new room
        if(roomStore[i].X > x+width || roomStore[i].x+roomStore[i].Width < x) continue;
        // roomStore[i] completly above or completly below new room
        if(roomStore[i].Y > y+height || roomStore[i].Y+roomStore[i].Height < y) continue;

        //if a room is neither completely to the right nor completely to the left as well as
        //not completly above nor completely below, it will intersect
        return false;
    }
    return true;
}


Using MySQL with Wai and Warp I was wondering if below is the correct convention for combining Wai with a Database pool.

What I basically do is, create a pool, partially apply a function of type Pool -> Application and use it to pass it to Warp's run. Does it look ok, or shall I refactor it?

{-# LANGUAGE OverloadedStrings #-}
import Network.Wai
import Network.HTTP.Types
import Network.Wai.Handler.Warp (run)
import Database.MySQL.Simple
import Data.Pool (Pool, createPool, withResource)

newConn = connect defaultConnectInfo
    { connectHost = "db"
    , connectUser = "root"
    , connectPassword = "secret"
    , connectDatabase = "test" }

getPool = createPool newConn close 1 10 5

app :: Pool Connection -> Application
app pool _ respond = do
    withResource pool $ \c -> query_ c "SELECT 1" :: IO [Only Int]
    respond $ responseLBS
        status200
        [("Content-Type", "text/plain")]
        "Hello, Web!"

main :: IO ()
main = do
    putStrLn $ "http://localhost:8080/"
    pool <- getPool
    run 8080 $ app $ pool

I'm afraid there isn't much to remove. The $ after putStrLn and between app and pool are superfluous. You could write the last two lines run 8080 . app <$> getPool. I'd inline getPool. You may be interested in https://github.com/alevy/simple/blob/master/simple/src/Web/Simple.hs#L134 for boilerplate reduction.


Java implementation of the caesar-cipher I've wrote a little program that encrypts text by using the caesar-cipher.
Also it contains a little GUI, created by using swing.
Here's the full code:

import javax.swing.JOptionPane;
import javax.swing.JScrollPane;
import javax.swing.JTextArea;
import java.awt.Dimension;
public class caesar {

    public static void main(String[] args) {
        String field, text;

        field = JOptionPane.showInputDialog("Please enter text:");
        field = field.replaceAll("[^a-zA-Z]+", "");
        field = field.toUpperCase();

        int shift;

        String shift_String = JOptionPane.showInputDialog("Please enter shift to the right:");
        shift = Integer.parseInt(shift_String);

        String d = JOptionPane.showInputDialog("Encrypt (1) or decrypt (2):");
        int decision = Integer.parseInt(d);

        String out;

        if(decision==1) {
            out = encrypt(field, shift);
            JTextArea msg = new JTextArea(out);
            msg.setLineWrap(true);
            msg.setWrapStyleWord(true);
            JScrollPane scrollPane = new JScrollPane(msg);
            scrollPane.setPreferredSize(new Dimension(300,300));
            JOptionPane.showMessageDialog(null, scrollPane);
        }
        if(decision==2) {
            out = decrypt(field, shift);
            JTextArea msg = new JTextArea(out);
            msg.setLineWrap(true);
            msg.setWrapStyleWord(true);
            JScrollPane scrollPane = new JScrollPane(msg);
            scrollPane.setPreferredSize(new Dimension(300,300));
            JOptionPane.showMessageDialog(null, scrollPane);
        }
    }

    //Encryption
    public static String encrypt(String text, int n) {
        int x = 0;
        int y = 0;
        String out = ""; //Empty string for result.
        while (x < text.length()) {
            if (text.charAt(x) > 64 && text.charAt(x) < 91) {
                if (text.charAt(x) + n > 90) {
                    y = 26;
                }
                out = out + (char) (text.charAt(x) + n - y);
            } else {
                out = out + text.charAt(x);
            }
            x++;
            y = 0;
        }
        return out;
    }

    //Decryption
    public static String decrypt(String text, int n) {
        int x = 0;
        int y = 0;
        String out = ""; //Empty string for result.
        while (x < text.length()) {
            if (text.charAt(x) > 64 && text.charAt(x) < 91) {
                if (text.charAt(x)-n < 65) {
                    y = 26;
                }
                out = out + (char) (text.charAt(x) - n + y);
            } else {
                out = out + text.charAt(x);
            }
            x++;
            y = 0;
        }
        return out;
    }
}

My question now is: How to improve this code?

I mean, it does what it is supposed to do, but it's not really great code.

In my opinion, the main problem with your code is the duplication, here is my advices.

1) Put the ui code out of the conditions.

The only issue there, if the choice is invalid, you can either show a default string, or throw an exception.

        if (decision == 1) {
            out = encrypt(field, shift);
        } else if (decision == 2) {
            out = decrypt(field, shift);
        } else {
            out = "Invalid choice!";
        }

        JTextArea msg = new JTextArea(out);
        msg.setLineWrap(true);
        msg.setWrapStyleWord(true);
        JScrollPane scrollPane = new JScrollPane(msg);
        scrollPane.setPreferredSize(new Dimension(300, 300));
        JOptionPane.showMessageDialog(null, scrollPane);

Or

        if (decision == 1) {
            out = encrypt(field, shift);
        } else if (decision == 2) {
            out = decrypt(field, shift);
        } else {
            throw new IllegalStateException("Invalid choice!")
        }

        JTextArea msg = new JTextArea(out);
        msg.setLineWrap(true);
        msg.setWrapStyleWord(true);
        JScrollPane scrollPane = new JScrollPane(msg);
        scrollPane.setPreferredSize(new Dimension(300, 300));
        JOptionPane.showMessageDialog(null, scrollPane);

2) In the encrypt & decrypt, to create the string containing the result, i suggest that you use java.lang.StringBuilder instead of concatening the String; you will gain some performance.

    public static String decrypt(String text, int n) {
        int x = 0;
        int y = 0;
        StringBuilder out = new StringBuilder(); //Empty string for result.
        while (x < text.length()) {
            if (text.charAt(x) > 64 && text.charAt(x) < 91) {
                if (text.charAt(x) - n < 65) {
                    y = 26;
                }
                out.append(text.charAt(x) - n + y);
            } else {
                out.append(text.charAt(x));
            }
            x++;
            y = 0;
        }
        return out.toString();
    }

3) In the encrypt & decrypt, extract the text.charAt(x) in a variable, to remove the duplicates.

    public static String decrypt(String text, int n) {
        int x = 0;
        int y = 0;
        StringBuilder out = new StringBuilder(); //Empty string for result.
        while (x < text.length()) {
            final char currentChar = text.charAt(x);

            if (currentChar > 64 && currentChar < 91) {
                if (currentChar - n < 65) {
                    y = 26;
                }
                out.append(currentChar - n + y);
            } else {
                out.append(currentChar);
            }
            x++;
            y = 0;
        }
        return out.toString();
    }

4) The encrypt and decrypt methods are pretty similar, you can probably merge them if you want.

    //Encryption
    public static String encrypt(String text, int n) {
        return operation(text, n, true);
    }

    //Decryption
    public static String decrypt(String text, int n) {
        return operation(text, n, false);
    }

    public static String operation(String text, int n, boolean isEncryption) {
        int x = 0;
        int y = 0;
        StringBuilder out = new StringBuilder(); //Empty string for result.
        while (x < text.length()) {
            final char currentChar = text.charAt(x);

            if (currentChar > 64 && currentChar < 91) {
                if (isEncryption ? (currentChar + n > 90) : (currentChar - n < 65)) {
                    y = 26;
                }
                out.append(isEncryption ? (currentChar + n - y) : (currentChar - n + y));
            } else {
                out.append(currentChar);
            }
            x++;
            y = 0;
        }
        return out.toString();
    }
```


Timetable app for myself My aim was to build a small, simple program that would show me my school timetable for the present day as well as the following day. I am new to PyQt(5) and programming in general. I started programming about 5 months ago on and off just as a fun side hobby. I started with basic text-based programs which gradually got more complex and this is my first (useful) GUI program. Any thoughts/suggestions?

import datetime
import sys
from PyQt5.QtCore import Qt
from PyQt5.QtWidgets import (QHBoxLayout, QApplication, QWidget, QLabel)
from PyQt5.QtGui import (QIcon, QPixmap)

#My weekly school timetable
Mon = '\n1       Spanish\n2       Spanish\n3       Music\n4       Music\n5       English Literature\n6       English Literature\n7       History\n8       History'
Tue = '\n1       Social Studies\n2       Social Studies\n3       Spanish\n4       Spanish\n5       Information Technology\n6       Information Technology\n7       English Literature\n8       English Language'
Wed = '\n1       English Literature\n2       English Literature\n3       Information Technology\n4       Information Technology\n5       HSB\n6       HSB\n7       Mathematics\n8       Mathematics'
Thu = '\n1       History\n2       History\n3       English Language\n4       English Language\n5       HSB\n6       HSB\n7       Mathematics\n8       Mathematics'
Fri = '\n1       Music\n2       Music\n3       English Language\n4       English Language\n5       Mathematics\n6       Mathematics\n7       Social Studies\n8       Social Studies'

#Returns day in terms of Monday = 0, Tuesday = 1...
day = datetime.datetime.today().weekday()

if day==0:
    the_day = Mon
    tomorrow = Tue
if day==1:
    the_day = Tue
    tomorrow = Wed
if day==2:
    the_day = Wed
    tomorrow = Thu
if day==3:
    the_day = Thu
    tomorrow = Fri
if day==4:
    the_day = Fri
    tomorrow = ''
elif day==5 or day==6:
    the_day = ''
    tomorrow = Mon

if day in range(5):
    today = '<b>Today\'s Timetable:<\b>'
    next_day = '<b>Tomorrow\'s Timetable:<\b>'
else:
    today = ''
    next_day = '<b>Monday\'s Timetable:<\b>'

class timetable(QWidget):

    def __init__(self):
        super().__init__()

        self.initUI()

    def initUI(self):
        if day==4:
            lbl = QLabel(self)
            pixmap = QPixmap('weekend.jpg')
            smaller_pixmap = pixmap.scaled(160, 300, Qt.KeepAspectRatio, Qt.FastTransformation)
            lbl.setPixmap(smaller_pixmap)
            lbl.move(-3, 162)
            lbl.show()

        elif day==5 or day==6:
            lbl = QLabel(self)
            pixmap = QPixmap('weekend.jpg')
            smaller_pixmap = pixmap.scaled(160, 300, Qt.KeepAspectRatio, Qt.FastTransformation)
            lbl.setPixmap(smaller_pixmap)
            lbl.move(-3, 0)
            lbl.show()

        title0 = QLabel(today, self)
        title0.move(5,5)

        schedule0 = QLabel(the_day, self)
        schedule0.move(5, 16)

        title1 = QLabel(next_day, self)
        title1.move(5,150)

        schedule1 = QLabel(tomorrow, self)
        schedule1.move(5, 160)

        self.setGeometry(7, 30, 150, 287)
        self.setFixedSize(self.size())
        self.setWindowTitle('Timetable')
        self.setWindowIcon(QIcon('icon.png'))
        self.show()

if __name__ == '__main__':

    app = QApplication(sys.argv)
    ex = timetable()
    ex.show()
    sys.exit(app.exec_())
    #created with PyQt5 using Python 3.5

I would change your setup code to take a list of lists, where the inner lists are lists of subjects on that day and the outer list collects all these lists:

schedule = [['Spanish', 'Spanish', 'Music', 'Music', 'English Literature', 'English Literature', 'History', 'History'],
            ...]

def format_day(day_schedule):
    return "\n" + "\n".join("{}       {}".format(i, subject)
                            for i, subject in enumerate(day_schedule, 1))

today = date.datetime.today().weekday()

try:
    the_day = format_day(schedule[today])
except IndexError:
    the_day = ''
try:
    tomorrow = format_day(schedule[today + 1])
except IndexError:
    tomorrow = format_day(schedule[0])

This could actually be moved into the class, which could just take the schedule and day as parameters (which would make it a lot easier to run this, e.g for two different schedules):

import datetime
import sys
from PyQt5.QtCore import Qt
from PyQt5.QtWidgets import (QHBoxLayout, QApplication, QWidget, QLabel)
from PyQt5.QtGui import (QIcon, QPixmap)

def format_day(day_schedule):
    return "\n" + "\n".join("{}       {}".format(i, subject)
                            for i, subject in enumerate(day_schedule, 1))

class Timetable(QWidget):

    def __init__(self, schedule, day):
        super().__init__()
        self.schedule = schedule
        self.day = day
        self.init_UI()

    def init_UI(self):
        try:
            today = format_day(self.schedule[self.day])
        except IndexError:
            today = ''
        try:
            tomorrow = format_day(self.schedule[self.day + 1])
        except IndexError:
            tomorrow = format_day(self.schedule[0])

        if self.day <= 5:
            today_header = '<b>Today\'s Timetable:<\b>'
            tomorrow_header = '<b>Tomorrow\'s Timetable:<\b>'
        else:
            today_header = ''
            tomorrow_header = '<b>Monday\'s Timetable:<\b>'

        if self.day in (4, 5, 6):
            lbl = QLabel(self)
            pixmap = QPixmap('weekend.jpg')
            smaller_pixmap = pixmap.scaled(160, 300, Qt.KeepAspectRatio, Qt.FastTransformation)
            lbl.setPixmap(smaller_pixmap)
            if self.day == 4:
                lbl.move(-3, 162)
            else:
                lbl.move(-3, 0)
            lbl.show()

        title0 = QLabel(today_header, self)
        title0.move(5, 5)
        schedule0 = QLabel(today, self)
        schedule0.move(5, 16)

        title1 = QLabel(tomorrow_header, self)
        title1.move(5, 150)
        schedule1 = QLabel(tomorrow, self)
        schedule1.move(5, 160)

        self.setGeometry(7, 30, 150, 287)
        self.setFixedSize(self.size())
        self.setWindowTitle('Timetable')
        self.setWindowIcon(QIcon('icon.png'))
        self.show()

if __name__ == '__main__':
    app = QApplication(sys.argv)

    schedule = [['Spanish', 'Spanish', 'Music', 'Music', 'English Literature', 'English Literature', 'History', 'History'],
                ...]
    today = date.datetime.today().weekday()
    ex = Timetable(schedule, today)
    ex.show()
    sys.exit(app.exec_())
    #created with PyQt5 using Python 3.5

Note that I also renamed some of your variables (there is now a today and a today_header and a tomorrow and tomorrow_header.


Java Perlin noise implementation I've decided to code Perlin noise generator in Java. It works pretty well, I just want to know If I am doing everything right and if it is a valid implementation!

PS : The metaVector2f and metaVector2i are just 2D vector classes

public float[][] generate(long seed, metaVector2i size, metaVector2i gridSize) {
    Random rand = new Random(seed);
    metaVector2f[][] gvectors = new metaVector2f[gridSize.x][gridSize.y];

    for (int y = 0; y < gridSize.y; y++)
        for (int x = 0; x < gridSize.x; x++)
            gvectors[x][y] = new metaVector2f(rand.nextFloat(),rand.nextFloat());

    float[][] data = new float[size.x][size.y];

    for (int y = 0; y < size.y; y++)
        for (int x = 0; x < size.x; x++) {
            data[x][y] = perlinPoint(new metaVector2f(x/(float)(size.x/gridSize.x),y/(float)(size.y/gridSize.y)), gvectors);
        }

    return data;
}

private float perlinPoint(metaVector2f position, metaVector2f[][] gvectors) {

    int x0 = (int)position.x;
    int y0 = (int)position.y;
    int x1 = (int)position.x+1;
    int y1 = (int)position.y+1;

    float dx0 = position.x-x0;
    float dy0 = position.y-y0;

    float dx1 = position.x-x1;
    float dy1 = position.y-y1;

    float h1 = dotProduct(new metaVector2f(dx0, dy0), gvectors[x0%gvectors.length][y0%gvectors[0].length]);
    float h2 = dotProduct(new metaVector2f(dx1, dy0), gvectors[x1%gvectors.length][y0%gvectors[0].length]);
    float fin1 = lerp(h1, h2, dx0);

    float v1 = dotProduct(new metaVector2f(dx0, dy1), gvectors[x0%gvectors.length][y1%gvectors[0].length]);
    float v2 = dotProduct(new metaVector2f(dx1, dy1), gvectors[x1%gvectors.length][y1%gvectors[0].length]);
    float fin2 = lerp(v1, v2, dx0);

    return lerp(fin1,fin2,dy0);

}

private float lerp(float x, float y, float t) {
    return (1.0f - t)*x + t*y;
}

private float dotProduct(metaVector2f vec, metaVector2f grad) {
    return vec.x*grad.x+vec.y*grad.y;
}

Here is the output (value/2+0.5f) :

Consider warmwaffle's code at GitHub (re: https://github.com/warmwaffles/Noise).  The one difference I see between the two solutions is how the 'metaVector2f's are created.  Your code is using non-repeatable random numbers; whereas, the other uses a mathematical calculation which includes prime numbers and bit shifting operations.  The latter would produce a repeatable solution with the same seed versus yours.  This may be worth something if you're testing or troubleshooting elsewhere.


Implementing JQuery style 'deferred' and 'promise' in C# I like the pattern of the jQuery Deferred object.

I like how you can call Resolve any number of times, but the listening objects will only be notified once. I also like how you can attach a listening object after Resolve has already been called, and the listening object will still get the notification. If you're programming in an uncertain multithreaded environment, it greatly reduces the amount of thinking you have to do.

I couldn't find an equivalent in the C# library, so here is my implementation (a basic subset so far):

using System;
using System.Collections.Concurrent;
using System.Threading;

interface IPromise
{
    void Done(Action action);
}

sealed class Deferred : IPromise
{
    readonly ConcurrentBag<Action> _actions = new ConcurrentBag<Action>();
    // using an int instead of a bool
    // so we can use Interlocked
    private int _isResolved = 0;

    internal void Resolve()
    {
        Interlocked.Exchange(ref _isResolved, 1);
        InvokeActions();
    }

    void IPromise.Done(Action action)
    {
        _actions.Add(action);
        if (_isResolved == 1)
        {
            InvokeActions();
        }
    }

    void InvokeActions()
    {
        Action action = null;
        while (_actions.TryTake(out action))
        {
            action();
        }
    }
}

For this code to be usable, it has to be completely 100% thread safe. Can anyone see a hole in it?

Also, am I just re-implementing something that already exists in some corner of the .NET base class library?

For the sake of context I'll provide a practical use: there's a long running application, and some code that must execute if a file exists. The file might exist when the code first runs, or might come to exist later, or might never exist at all.
I invoke it like this:

 string fullPath = GenerateWatchedFilePath();
 FileSystemWatcherUtils
      .WhenFileAppears(fullPath)
         .Done(() =>
                {
                    using (var stream = File.OpenRead(fullPath))
                    {
                       // etc
                    }
                }
              );

And my implementation of WhenFileAppears:

public static IPromise WhenFileAppears(string fullPath)
{
   Deferred returnPromise = new Deferred();
   if (File.Exists(fullPath))
   {
      returnPromise.Resolve();
      return returnPromise;
   }
   string subFolder = Path.GetDirectoryName(fullPath);
   // This code assumes the subFolder exists
   // An exception will be thrown if it does not
   var fileSystemWatcher = new FileSystemWatcher(subFolder);
   int onceOnlyFlag = 0;
   Action disposeOfWatcherAndResolve = () =>
   {
      if (Interlocked.Exchange(ref onceOnlyFlag, 1) == 0)
      {
          fileSystemWatcher.Dispose();
          returnPromise.Resolve();
      }
   };
   FileSystemEventHandler watcherHandler = (sender, args) =>
     {
        if (args.FullPath == fullPath)
        {
           disposeOfWatcherAndResolve();
        }
     };
   fileSystemWatcher.Created += watcherHandler;
   fileSystemWatcher.Renamed += (sender, args) => watcherHandler(sender, args);
   fileSystemWatcher.EnableRaisingEvents = true;
   // Dealing with a race condition:
   // if the file came to exist between the last check
   // and the creation of the fileSystemWatcher
   if (File.Exists(fullPath))
   {
      disposeOfWatcherAndResolve();
   }
   return returnPromise;
}

I like how you can call Resolve any number of times, but the listening objects will only be notified once. I also like how you can attach a listening object after Resolve has already been called, and the listening object will still get the notification. If you're programming in an uncertain multithreaded environment, it greatly reduces the amount of thinking you have to do.

This sounds a lot like TaskCompletionSource along with the Task it controls.

Assuming you have a TaskCompletionSource<T> tcs:

To attach a listener, use tcs.Task.ContinueWith(_ => /* listener code here */). You can do this before or after the Task has been completed and the listener code will be executed only once after the Task has been completed.
To complete the Task (the equivalent of resolving), use tcs.TrySetResult(someValue). If you'll use the TrySetResult() variant instead of SetResult(), the second and following attempt at completing will be ignored (SetResult() would throw an exception).

The difference with your code is that TaskCompletionSource always has some result type. You can either just ignore that (using e.g. TaskCompletionSource<object> and SetResult(null)), or use the non-generic TaskCompletionSource from AsyncEx.


NMEA 0183 checksum validation I was hoping for a bit of advice on cleaning a couple of areas of this code up. In particular, the hex-to-binary section, the checksum is in hex, and the rest is in ASCII. I tried for a good day to do it with tables etc, but in frustration I ended up going with this.

Is there a better way of handling the "Store individual characters in vector" section? I went that way because the message can be of variable size (plus 1 or 2 characters).

Here's an example of the type of message:

!AIVDM,1,1,,A,13HOI:0P0000VOHLCnHQKwvL05Ip,0*23

using namespace std;

int main()

{
//Checksum Function

//Get Characters for Checksum
ifstream AisMsg("nmea.txt", ios::in);

    string MsgLine;
    getline(AisMsg, MsgLine, '*');
    MsgLine.erase(0,1);

        string HexVal;
        getline(AisMsg, HexVal);

AisMsg.close();

//Store individual characters in an element of an vector
int i = 0;
int j = ( MsgLine.length() );
vector <int> AsciiValues;
{
    bool done = false;
    while (!done)
    {
        int h ( MsgLine.at (i) );
        i++;
        AsciiValues.push_back(h);

    if ( i == j )
        done = true;
    }
}
//XOR validation
bitset<8> a = (AsciiValues[0]);
bitset<8> b = (AsciiValues[1]);
bitset<8> c = a ^ b;
    int d = 2;
    int e = (AsciiValues.size());

{
    bool done = false;
    while (!done)
    {
        a = (AsciiValues[d]);
        b = c;
        c = a ^ b;
        d++;

    if ( d == e )
        done = true;
    }
}
//Hex Checksum to Binary Conversion (messy, but does the job)
int PosOne = HexVal.at(0);
int PosTwo = HexVal.at(1);

    bitset<4> z = PosOne;
    bitset<4> x = PosTwo;

        ofstream Cheat("Amateur Hour.txt", ios::out | ios::app);
        Cheat << z << x << endl;
        ifstream Cheat1("Amateur Hour.txt", ios::in | ios::binary);

            string Chksm;
            getline(Cheat1,Chksm);

        Cheat.close();
        Cheat1.close();
        remove ("Amateur Hour.txt");

return 0;
}

I believe this can be simplified quite a bit. My immediate reaction would be to write code something like this:

#include <sstream>
#include <iostream>
#include <numeric>

int main() {
    std::istringstream in("$AIVDM,1,1,,A,13HOI:0P0000VOHLCnHQKwvL05Ip,0*23");

    std::string sentence;

    std::getline(in, sentence, '*');

    std::string received_checksum;

    // Save the checksum that was at the end of what we received.
    std::getline(in, received_checksum);

    // Re-compute the check-sum:
    char check = std::accumulate(sentence.begin()+1, sentence.end(), 0,
        [](char sum, char ch) { return sum ^ ch; });

    // Print out both the received and computed checksums:
    std::cout << "received checksum: " << received_checksum;
    std::cout << "\ncomputed checksum: " << std::hex << (int)check << "\n";
}

Looking at specific details of the code, I see a few things that even if you were going to preserve roughly your current implementation, you could still simplify and clarify. The first point would be to fix your indentation. Right now, it appears to be almost entirely arbitrary, and unrelated to the structure of the code.

Then you could clean up some of your loops. For example:

bool done = false;
while (!done)
{
    int h ( MsgLine.at (i) );
    i++;
    AsciiValues.push_back(h);

if ( i == j )
    done = true;
}

...can be turned into something like this:

for (int i=0; i<j; i++)
    AsciiValues.push_back((int)MsgLine.at(i));

Or, you could just use std::copy. Likewise, your code for computing the checksum:

bool done = false;
while (!done)
{
    a = (AsciiValues[d]);
    b = c;
    c = a ^ b;
    d++;

if ( d == e )
    done = true;
}

...could be simplified down to something like this:

do {
    a = AsciiValues[d];
    b = c;
    c = a ^ b;
    ++d;
} while (d != e);

...or you could do something closer to the accumulate (it still seems a lot cleaner to me to use std::accumulate), as I showed to start with.


Basic calculator in Swift 3 I just started learning the basics to Swift 3 about 2 days ago. Since then, I've decided to make a simple calculator that can only do one operation at a time.

I was wondering how efficient this was and what I could possibly do to make it more efficient. Any type of criticism would be helpful too

import UIKit

class ViewController: UIViewController {

    @IBOutlet var buttonResponseLabel: UILabel!
    var userIsCurrentlyTyping: Bool = false
    var storedValue: Int = 0;
    var currentOperation = 0
    var currentNumberInLabel = ""

    @IBAction func buttonAppend(_ sender: UIButton)
    {
        let buttonTitle = sender.currentTitle!

        switch buttonTitle
        {
            case "+":
            currentOperation = 1
            storedValue = Int(currentNumberInLabel)!
            buttonResponseLabel.text = ""
            case "-":
            currentOperation = 2
            storedValue = Int(currentNumberInLabel)!
            buttonResponseLabel.text = ""
            case "*":
            currentOperation = 3
            storedValue = Int(currentNumberInLabel)!
            buttonResponseLabel.text = ""
            case "/":
            currentOperation = 4
            storedValue = Int(currentNumberInLabel)!
            buttonResponseLabel.text = ""
            default:

            if buttonTitle == "="
            {
                var finalValue = 0
                print(currentNumberInLabel)
                let secondStoredValue = Int(currentNumberInLabel)!
                if currentOperation == 1
                {
                    finalValue = storedValue + secondStoredValue
                    buttonResponseLabel.text = ""
                    buttonResponseLabel.text = String(finalValue)
                }
                else if currentOperation == 2
                {
                    finalValue = storedValue - secondStoredValue
                    buttonResponseLabel.text = ""
                    buttonResponseLabel.text = String(finalValue)
                }
                else if currentOperation == 3
                {
                    finalValue = storedValue * secondStoredValue
                    buttonResponseLabel.text = ""
                    buttonResponseLabel.text = String(finalValue)
                }
                else if currentOperation == 4
                {
                    finalValue = storedValue / secondStoredValue
                    buttonResponseLabel.text = ""
                    buttonResponseLabel.text = String(finalValue)
                }
            }
            else
            {
                if userIsCurrentlyTyping
                {
                    currentNumberInLabel = buttonResponseLabel.text! + buttonTitle
                    buttonResponseLabel.text = currentNumberInLabel
                }
                else
                {
                    userIsCurrentlyTyping = true
                    buttonResponseLabel.text = ""
                    buttonResponseLabel.text = buttonTitle
                }

            }

            print("\(currentOperation) \(buttonTitle)")
        }

    }

}

Try not to repeat yourself and refactor common code to functions. Also you might want to create types (classes, structs, enums) to make things more expressive. Quick example with not so nice new data type (untested):

import UIKit

class ViewController: UIViewController {

    struct Operator {
        var stringRepresentation: String
        var `operator`: (Int, Int) -> Int //quotes required  as operator is a keyword
    }

    var operators = ["+":Operator(stringRepresentation:"+", operator:+),
                 "-":Operator(stringRepresentation:"-", operator:-),
                 "*":Operator(stringRepresentation:"*", operator:*),
                 "/":Operator(stringRepresentation:"/", operator:/)]
    @IBOutlet var buttonResponseLabel: UILabel!
    var userIsCurrentlyTyping: Bool = false
    var storedValue: Int = 0;
    var currentOperation: Operator?
    var currentNumberInLabel = ""
    var buttonTitle = ""

    @IBAction func buttonAppend(_ sender: UIButton) {
        buttonTitle = sender.currentTitle!

        if let chosenOperator = operators[buttonTitle] {
            currentOperation = chosenOperator
            storedValue = Int(currentNumberInLabel)!
            buttonResponseLabel.text = ""
        } else if buttonTitle == "=" {
            showResult()
        } else if userIsCurrentlyTyping {
            currentNumberInLabel = buttonResponseLabel.text! + buttonTitle
            buttonResponseLabel.text = currentNumberInLabel
        } else {
            userIsCurrentlyTyping = true
            buttonResponseLabel.text = buttonTitle
        }

        print("\(currentOperation) \(buttonTitle)")
    }

    func showResult() {
        print(currentNumberInLabel)
        guard let currentOperator = currentOperation else {
            print("This should never happen. Fail silently")
            return
        }
        operate(withOperator: currentOperator.operator)
    }

    func operate(withOperator op: (Int, Int) -> Int){
        let secondStoredValue = Int(currentNumberInLabel)!
        let finalValue = op(storedValue, secondStoredValue)
        buttonResponseLabel.text = String(finalValue)
    }
}

A very good walk-through Swift by implementing a calculator can be found here btw. It's from late 2015, so no Swift3. Maybe you find an update.


Making line by line replacements using values from multiple arrays I currently have a Python script that parses a file (in this case an XML file) and makes line by line replacements as necessary, depending on the values of multiple arrays. As it stands now the script works great, but feel like it could be a lot better. I'll also need to add few more arrays later on and am worried about performance.

Additionally, the way I currently have it set up doesn't allow for counting and printing the total replacements that have been made. (e.g. "Replaced: xyz -- Made XX replacements") While not an immediate requirement, I'd like to add this functionality in the future.

arrayOne = ["old string one", "new string one"]
arrayTwo = ["old string two", "new string two"]

# Variable 'path' collected from command line input
f = open(path, "r", encoding="utf8")
newFile = open(path.replace(".xml", "-new.xml"), "w", encoding="utf8")

def replace(a,b):
    for data in f:
        for datatype in (arrayOne, arrayTwo):
            data = data.replace(datatype[a], datatype[b])
        newFile.write(data)
    newFile.close()

replace(0,1)

f.close()

One bug I see is that you perform the string substitutions sequentially, rather than "simultaneously".  For example, suppose you had an input file with contents

abc

with substitution code

arrayOne = ["a", "b"]
arrayTwo = ["b", "c"]

…
replace(0, 1)

Most users would expect the output to be bcc.  However, your code actually produces ccc, which would probably be considered surprising.

The main problem, though, is that everything is tightly coupled, such that the code is not reusable.

For example, replace() is a closure that captures arrayOne and arrayTwo, as well as file f.  It doesn't generalize to perform one substitution or ≥ 3 substitutions.  The two parameters that it does accept are more annoying than helpful, as you have indicated that you expect to always call it as replace(0, 1).  Furthermore, replace() will only operate on the file that was opened as f at the time that the function was defined.

A better interface would be for replace() to accept a dict of substitutions to be performed.  Furthermore, it should just concern itself with string transformation, staying clear of all file operations.

I propose the following solution, which contains some rather advanced concepts.

from functools import partial
import re

def str_replacer(mapping):
    regex = re.compile('|'.join(re.escape(orig) for orig in mapping))
    return partial(regex.sub, lambda match: mapping[match.group(0)])

You would use it like this:

>>> replace = str_replacer({
...     'a': 'b',
...     'b': 'c',
...     'c': 'd',
... })
>>> print(replace('abc'))
bcd

Explanation: Performing "simultaneous" replacements means scanning the input string until any of the "old" strings is encountered.  If a substitution is performed, the scanning should recommence at the after where the newly spliced-in string ends.

That's tedious to do using low-level string manipulation, so I've opted to use re.sub() instead.  To use re.sub(), I have to do two things

Compose a regular expression that includes all of the "old" strings to look for.  That's what re.compile(…) does.
Create a callback function that specifies what the replacement for each old string should be.  That's the lambda.  match.group(0) is the old string that was found.  We look its corresponding "new" string in the mapping.

However, I don't want str_replacer() to call regex.sub(repl, input_string) just yet.  Instead, I want str_replacer() to return a function, that if called, performs the desired replacements.  Why? Because you will likely call the resulting function many times if you want to process a file a line at a time, and it would be more efficient to reuse the regex and the lambda.  That's what functools.partial() does: it creates a function that accepts an input string; when that function is called, then the regex.sub(lambda match: mapping[match.group(0)], input_string) actually happens.

replace = str_replacer({
    'old string one': 'new string one',
    'old string two': 'new string two',
})

with open(path, "r", encoding="utf8") as in_file, \
     open(path.replace(".xml", "-new.xml"), "w", encoding="utf8") as out_file:
    for line in in_file:
        out_file.write(replace(line))

Now the string replacement logic and the file manipulation logic are cleanly separated!


Vending Machine class I am looking for ways to improve the code that I wrote for this.

  Consider a simple vending machine class. The machine accepts tokens
  and dispenses cans of refreshing beverages. Write a complete class (on
  the next page) as described below:

  The class has two instance data fields; one to keep track of the number of cans in the machine and one to keep track of the number of
  tokens (think coins) collected.
  There should be two constructors. One takes no arguments and starts with 50 cans and zero tokens. The other takes one argument, the
  initial number of cans in the machine.
  There should be a method to purchase a drink which adds one token to the machine and subtracts one can – assuming there are still cans in
  the machine to be purchased.
  There should be one method to add cans to the machine.
  There should be separate methods to return the number of cans remaining and the number of tokens collected.
  Write a toString( ) method to allow for easy printing of a vending machine object

public class Vendingmachine
{
    private int tokens;
    private int cans;

    public Vendingmachine()
    {
        tokens=0;

        cans=50;
    }

    public Vendingmachine( int ca)
    {
        tokens=0;

        cans=ca;

        System.out.printf(" The constructor for this %s\n", this);
    }
    public void setTokens( int coins)
    {
            tokens = coins;
    }

    public void setCans ( int ca)
    {
        cans = ca;
    }

    public void purchase( int coins)
    {
        tokens = coins;
        if (tokens >=1)
            cans = cans-tokens;
        if (cans == 0)
        addCans();
    }

    public int getTokens()
    {
        return tokens;
    }

    public int getCans()
    {
        cans = cans- tokens;
        return cans;
    }

    public void addCans()
    {

            cans =50;
    }

    public String toString()
    {
    String str = "You have purchased "+ cans  + " cans and have entered the following amount of coins: " + tokens;
    return str;
    }

}

I've noted a few things you could consider:

Your class should probably be VendingMachine, note the capital M.
The value 50 is a magic number and should probably be defined as a const at the top of your class.
The use of coins and tokens is confusing as they appear to refer to the same thing. Pick one term and use it everywhere.
getCans() changes the value of cans. This is a side-effect, which is generally considered a no-no, especially in a getter.
The Java idiom for decrementing is -=, so I'd change cans = cans - tokens; to cans -= tokens;
Both constructors contain similar code. I'd change the first to use the second, thus:

public Vendingmachine() {
    this(50);
}


Badge Oneboxer for Chat.SE This script makes it possible for badges to onebox in Chat.

The install requires Tamper/GreaseMonkey. Click here to install,
and it's on GitHub and StackApps!

// ==UserScript==
// @name Badge Oneboxer
// @description Converts tag-like badge comments to badges
// @version 0.24
// @match *://chat.stackexchange.com/rooms/*
// @match *://chat.stackoverflow.com/rooms/*
// @match *://chat.meta.stackexchange.com/rooms/*
// @author The-Quill
// @downloadURL  https://raw.githubusercontent.com/The-Quill/badge-oneboxer/master/badge-oneboxer.user.js
// @updateURL https://raw.githubusercontent.com/The-Quill/badge-oneboxer/master/badge-oneboxer.user.js
// @grant GM_getResourceText
// @resource    badges  https://rawgit.com/The-Quill/badge-oneboxer/master/badges.json
// @run-at document-end
// ==/UserScript==

    'use strict';
    var Badges;
    $.get("https://rawgit.com/The-Quill/badge-oneboxer/master/badges.json", function(data){
        Badges = data;
    }).done(function(){
        var m = new MutationObserver(function(){
            ReplaceAll();
        });
        m.observe(document.getElementById("chat"), {childList: true});
        ReplaceAll();
    });
    function ReplaceAll(){
        var messages = document.getElementsByClassName('message');
        for (var i = 0, length = messages.length; i < length - 1; i++){
            ReplaceText(messages[i])
        }
    }
    var regex = /(\[badge:([a-zA-Z#.\-]+)\])/i;
    var ColourTransforms = {
        bronze: "CC9966",
        silver: "C5C5C5",
        gold: "FFCC00"
    };
    function ReplaceText(node){
        if (!node) return false;
        var badgeProperties = SelectBadgeProperties(node.innerText);
        if (!badgeProperties) { return false };
        if (badgeProperties.total == "") return false;
        node.innerHTML = node.innerHTML.replace(
            badgeProperties.total,
            "" +
            "<svg version=\"1.1\" height=\"18\" width=\"15\" xmlns=\"http://www.w3.org/2000/svg\"><circle fill=\"#" +
            (Badges.hasOwnProperty(badgeProperties.name) ? ColourTransforms[Badges[badgeProperties.name]] : "000") +
            "\" r=\"3\" cy=\"13.5\" cx=\"3\" /></svg>" +
            badgeProperties.name + ""
        );
        if (HasBadgeText(node.innerText)){
            console.log("again");
            ReplaceText(node);
        }
    };
    function HasBadgeText(text){
        return regex.test(text);
    }
    function SelectBadgeProperties(text){
        if (!HasBadgeText(text)) return false;
        var matchesArray = text.match(regex);
        return {
            total: matchesArray[0],
            name:  matchesArray[2]
        };
    }

N.b. the current GitHub source already has some of these fixed.

Firstly, all lines are indented one level more than I would expect (the
first comment is also indented in the GitHub source); that seems to
break the user script annotation, as at the moment I don't see either
the description, the name, or the @match restrictions applied after
installation.

Otherwise formatting is fine, though the naming convention (upper camel
case for top level definitions) is unusual.  My editor also marks a few
lines with missing semicolon; that could be fixed just for consistency.

Even though it might not happen usually I'd ensure that all referenced
functions are defined before calling them, in this case ReplaceAll is
run from the callback, when it's only defined a few lines later.

The function ReplaceAll also returns false in a couple of cases, but
firstly, the return value isn't used anywhere and secondly, the function
doesn't return a value in one case.  I'd suggest to either always return
a value, or not at all.

The left over console.log should likely be deleted as it doesn't give
the reader much information.

In SelectBadgeProperties the regex match is done twice; it would make
more sense to only do the match and distinguish between the null
value and an actual match.

A minor gripe with ColourTransforms is that the starting # is left
out of the colour definition - this means that some external editors,
plugins, etc. won't recognize it as a colour.  I'd leave it in, in the
interest of compatibility and readability, even if it takes three
additional characters in the source.

And lastly, inserting many copies of the SVG image doesn't sound
performant, but I can't back that up with data obviously; I also doubt
that people will insert a massive number of badges and therefore this
point is kinda moot.

Otherwise looks great to complement the chat functionality.


Sanitize file or folder names to work with a legacy system I'm in the unfortunate position to where I have to create files and folders programmatically that have to work with a legacy system. In this process, the file and folder names have to be heavily sanitized.

The POST requests that create the files and folders are sent from the client in javascript, while the actual creation happens on a server. For this, I wrote a function used to display to users what their input folder or file names will look like and to sanitize their input to work with the legacy system.

I've let this run over quite a few test cases including irregular file names that broke the legacy system prior and it seems to work fine. I'm curious about what could be improved though, especially with respect to readability and extensibility.

/**
 * @description sanitizes a folder or file name to work with #name of legacy system
 * @author me
 * @param {string} object_name
 * @param {string} mode     "file" / "folder"
 */

function sanitizeFolderOrFile(object_name, mode) {

    if (mode !== 'file' && mode !== 'folder') {
        return false;
    }

    let suffix = '';
    let retvar = '';

    if (mode === 'file') {
        suffix = object_name.split('.').pop();
        retvar = object_name.replace(new RegExp('(\.' + suffix + ')', 'g'), '');
    } else if (mode === 'folder') {
        retvar = object_name;
    }

    // values to replace
    const replace_values = [{
        value: '+\\&',
        replacement: 'u'
    }, {
        value: 'äÄ',
        replacement: 'ae'
    }, {
        value: 'öÖ',
        replacement: 'oe'
    }, {
        value: 'üÜ',
        replacement: 'ue'
    }, {
        value: 'ß',
        replacement: 'ss'
    }, {
        value: ' \\`\\´\\?\\(\\)\\[\\]\\{\\}\\/\\\\$\\§\\"\\\'\\!\\=\\-\\.\\,\\;\\:<>\\|\\^\\°\\*\\+\\~\\%',
        replacement: '_'
    }];

    replace_values.forEach(function(element) {
        retvar = retvar.replace(new RegExp('([' + element.value + '])', 'g'), element.replacement);
    });

    retvar = retvar.toLowerCase();

    if (mode === 'file') {
        retvar += '.' + suffix;
    }

    return retvar;
}

I think you need separate some parts of code.

If you use ES5 with Decorators you can todo general determination Errors, and add for instance, function-condition in this Decorator.
You can apply mechanismes Extract Methods, Simplifying Conditional Expressions and Replace Magic Number with Symbolic Constant from sanitizeFolderOrFile.

/* Decorator file */
 function determinationError(condition) {
   return function (target: any, propertyKey: string, descriptor: PropertyDescriptor) {
    const method = descriptor.value;
    descriptor.value = function () {
      try {
         condition.apply(this, arguments);
         method.apply(this, arguments);
      }
      catch(e) {
        console.error(e.message);
      }
    };
    return descriptor;
  };
}

//export determinationError;

/*enum MODE file */
 const MODE = {
     FILE: 'file',
     folder: 'folder'
  };

//export MODE;

/*file sanitizeFolderOrFile */

//determinationError = require('determinationError'); // require our decorator file
//MODE = require('MODE'); // require our MODE file

function isFileOrFolderCondition(mode) {
  if (mode !== MODE.FILE && mode !== MODE.FOLDER) {
    throw new Error('It is not file or folder') ;
  }
}

  const replaceValues = [{
        value: '+\\&',
        replacement: 'u'
    }, {
        value: 'äÄ',
        replacement: 'ae'
    }, {
        value: 'öÖ',
        replacement: 'oe'
    }, {
        value: 'üÜ',
        replacement: 'ue'
    }, {
        value: 'ß',
        replacement: 'ss'
    }, {
        value: ' \\`\\´\\?\\(\\)\\[\\]\\{\\}\\/\\\\$\\§\\"\\\'\\!\\=\\-\\.\\,\\;\\:<>\\|\\^\\°\\*\\+\\~\\%',
        replacement: '_'
    }];

function replacerAndToLowerCase(name) {
   replaceValues.forEach(function(element) {
        name = name.replace(new RegExp('([' + element.value + '])', 'g'), element.replacement);
    });

   return name.toLowerCase();
}

function sanitizeFile(fullName) {
   const ext = fullName.split('.').pop();
   let name = fullName.replace(new RegExp('(\.' + ext + ')', 'g'), '');
   name = replacerAndToLowerCase(name);

   return `${name}.${ext}`;
}

function sanitizeFolder(fullName) {
   return replacerAndToLowerCase(fullName);
}

//@determinationError(isFileOrFolderCondition)
function sanitize(mode, name) {
   if (mode === MODE.FILE) {
     return sanitizeFile(name);
   }

   return sanitizeFolder(name);
}

const fileName = sanitize(MODE.FILE, 'TeSäÄtTE.txt');
console.log('fileName', fileName);

const folderName = sanitize(MODE.FOLDER, 'SuperFolder???????');
console.log('folderName', folderName);

I added comment in Snippet when you can add Decorator and separate on files