C++ custom exception design I have a custom exception hierarchy in C++ to detect unexpected cases in a library I am implementing. The base exception class inherits from std::exception and all other exception classes derive from this base class.

The header file for the base exception class is as follows:

#ifndef LibException_H_
#define LibException_H_

#include <exception>

class LibException : public std::exception
{
  public:
  // Sets member_err to err
  LibException(const char* err);

  virtual ~LibException() throw ();

  // Returns member_err.
  virtual const char* what() const throw();

  protected:
  // A description of the error. Should probably set this to private.
  const char* member_err;

  private:
};

#endif

And here is the header file for one of the derived classes, which is supposed to catch any process related error (I am spawning multiple processes within C++ in the source code, and I am using this class there for error handling):

#ifndef ProcessException_H_
#define ProcessException_H_

#include "Includes/LibException.H"

class ProcessException : public LibException
{
  public:
  enum Reason
  {
    NETWORK,
    IO,
    OTHER,
  };

  // Calls superclass(err) and sets the two member variables
  ProcessException(const char* err, Reason reason,
      const char* process_command);

  virtual ~ProcessException() throw ();

  // Returns a char* pointer to the reason.
  //
  // E.g. NETWORK = "NETWORK"
  // @return: <const char*> member_reason.
  const char* getReasonChar() const throw();

  // Returns a char* pointer to the spawned process' command (e.g. "ls -a").
  //
  // @return: member_command.
  const char* getCommand() const throw();

  protected:

  private:
  Reason member_reason;
  const char* member_command;
};

So one would invoke an error case for the derived class above by writing throw ProcessException("Process exited unexpectedly", ProcessException::NETWORK, process_command); where process_command is the string representation for a certain process (e.g. ls -a).

Does this implementation make sense? How can the above design be improved?

Edit: Moreover I have another exception class called ParseException (very similar to the derived class above) which is thrown when a parser that is parsing a file or a command from the shell fails unexpectedly. I have read that such errors are better handled via assert statements because they don't represent "exceptional" cases. Could anyone please comment on this?

You should probably inherit from std::runtime_error rather than std::exception.
The std::exception and all its descendants already implement what() so there is no need to implement that in your code. So you can remove these:
  // Returns member_err.
  virtual const char* what() const throw();

  protected:
  // A description of the error. Should probably set this to private.
  const char* member_err;

There is no need for a virtual destructor. That is already taken care of in the base class std::exception. Since you are not doing memory management (your example shows string literals) these methods are empty anyway. So you can remove this:
  virtual ~LibException() throw ();
  virtual ~ProcessException() throw ();

This seems obtuse:
  // Returns a char* pointer to the reason.
  //
  // E.g. NETWORK = "NETWORK"
  // @return: <const char*> member_reason.
  const char* getReasonChar() const throw();

Why not just return Reason?
I personally don't like interfaces that take pointers. There is no ownership semantics associated with the pointer and thus people can mistakenly provide the wrong type of pointer (ie a dynamically allocated one in this case). So I would change the interface to be std::string const& this guarantees the correct semantics are used (and you can still use the string literal). This also jibes with the std::exception (and family) which all accept a std::string for the constructor.
LibException(std::string const& err);

Edit: Moreover I have another exception class called ParseException (very similar to the derived class above) which is thrown when a parser that is parsing a file or a command from the shell fails unexpectedly.

I think the key phrase here is fails unexpectedly. If its unexpected to fail for this reason then its an exception.

I have read that such errors are better handled via assert statements because they don't represent "exceptional" cases.

Asserts are the worst type of control and they are really only good for debugging; not for error handling. Remember assert() compiles to the NO-OP when in release mode so there is no checks.

Could anyone please comment on this?

Control flow is usually not best handled with exceptions. But everything is situational. If you find it useful feature to use then go ahead.
Creating your Own exceptions
I used to create exceptions for different situations but found I never used them. So I have started simply using std::runtime_error. The only time I define a specific exception is when it can be usefully caught outside the library layer I am throwing from.
The trouble is the definition of "usefully caught". This not only means that the exception needs to carry information that I can use to fix the problem but the library itself must provide a public API that will allow me to fix the situation.
If all you can do with the exception is log it (including dump to std::cout). Then is it really a useful exception to catch?


Writing binary data to file in Linux I'm trying to learn C++ and I would interested in how I could write better code. The code should be independent of the architecture it is running on. What I want to do is writing some binary data to /dev/rfkill so that I can change some settings. The target is an embedded system so the program does run with root privileges and the program is using the Qt framework.

This is the code I currently have and which does what I want:

    rfkill_event event;
    event.type = RFKILL_TYPE_ALL;
    event.op = RFKILL_OP_CHANGE_ALL;
    event.soft = RFKILL_ACTION_UNBLOCK;

    QFile file("/dev/rfkill");
    if (!file.open(QIODevice::WriteOnly))
        quit();

    QDataStream out(&file);
    if (-1 == out.writeRawData((char*)(&(event.idx)), sizeof(event.idx))) {
        file.close();
        quit();
    }
    if (-1 == out.writeRawData((char*)(&(event.type)), sizeof(event.type))) {
        file.close();
        quit();
    }
    if (-1 == out.writeRawData((char*)(&(event.op)), sizeof(event.op))) {
        file.close();
        quit();
    }
    if (-1 == out.writeRawData((char*)(&(event.soft)), sizeof(event.soft))) {
        file.close();
        quit();
    }
    if (-1 == out.writeRawData((char*)(&(event.hard)), sizeof(event.hard))) {
        file.close();
        quit();
    }
    file.close();
    quit();

and the struct looks like this:

struct rfkill_event {
    __u32 idx;
    __u8  type;
    __u8  op;
    __u8  soft, hard;
};

The code is taken and adapted from the rfkill program.

I've removed __attribute__((packed)) from the struct because of the pitfalls mentioned here.

I'm using the writeRawData function instead of the << operator to write the data. The reason for this is that this code should work on big and little endian machines.

Are my assumptions correct and do you have any suggestions on how I could improve this code (besides not having any error handling)?

First I would like to say that I'm not an expert on rfkill; so my comments are more general and may be slightly off for this specific case.

Despite the missing error handling (which you mention) I see only two problems. Not primarily with your code, but with your assumptions:

You are correct to avoid __attribute__((packed)), but the reasons are IMHO not correct. The main reasons in your linked document are unaligned access, data size and performance. None of this is really a showstopper for your code, because you are breaking the data up into its primitive types. More significant is IMHO the simple fact that __attribute__((packed)) is not portable. (Unless you remain on GCC only. On a microsoft compiler you would write #pragma pack(1))
I think you are wrong using the writeRawData() function. Because your code should handle both, big endian and little endian, you would want to use the streaming << operator. I interpret the documentation like this: writeRawData() will always write in the machine byte order - big, or little endian - whatever the machine is. The streaming operators on the other hand will do a conversion, when necessary, to whatever endianness you have selected with setByteOrder().

There is another advantage using the streaming operators, you do not need to do ugly type casting. Instead of this:

out.writeRawData((char*)(&(event.idx)), sizeof(event.idx))

you would simply write:

out << event.idx;

Update: Error handling

I disagree with user1118321. I don't think that the QDataStream throws if it cannot write. I did not find anything about it in the documentation. (but the Qt documentation is known for having white spots) To handle write errors you can use the status() function. Or perhaps the functions from the underlying QIODevice.

out.resetStatus();
out << event.idx;
if(out.status() != QDataStream::Ok)
{
    // error handling
}


How can I efficiently manipulate digits in a string without using RegEx in Java? I am reading a book with different quizzes about coding interviews.
Please implement a function that increments a string based on the rules below:

It should take the string of unknown length and increment the numberic ending of that string by 1.
If numberic ending is overflown it must be reset.
Don't use regular expressions

Examples of correct functionality are the following:
assertEquals("000003", increment("000002"));
assertEquals("000000", increment("999999"));
assertEquals("GL-322", increment("GL-321"));
assertEquals("GL-000", increment("GL-999"));
assertEquals("DRI000EDERS1RE", increment("DRI000EDERS0RE"));
assertEquals("DRI000EDERS0RE00000", increment("DRI000EDERS0RE99999"));

Here is my solution, I am sure it could be vastly more efficient / succinct:
public class Increment {

    static String padLeftZeros(String inputString, int length) {
        if (inputString.length() >= length) {
            return inputString;
        }
        StringBuilder sb = new StringBuilder();
        while (sb.length() < length - inputString.length()) {
            sb.append('0');
        }
        sb.append(inputString);

        return sb.toString();
    }

    static String increment(String referenceNumber) {

        // Finding the last digit index in referenceNumber
        int indexLastDigit = -1;
        for (int i = 0; i < referenceNumber.length(); i++){
            try {
                Integer.parseInt(String.valueOf(referenceNumber.charAt(i)));
                indexLastDigit = i;
            } catch (NumberFormatException nfe) {
            }
        }

        // Finding the first digit index of the last digit group in referenceNumber
        int indexFirstDigit = -1;
        for (int i = indexLastDigit; i >= 0; i--){
            try {
                Integer.parseInt(String.valueOf(referenceNumber.charAt(i)));
                indexFirstDigit = i;
            } catch (NumberFormatException nfe) {
                break;
            }
        }

        // Checking if numberToIncrement needs reset or not
        String numberToIncrement = referenceNumber.substring(indexFirstDigit, indexLastDigit + 1);
        boolean isReset = true;
        for (int i = 0; i < numberToIncrement.length(); i++){
            if (numberToIncrement.charAt(i) != '9') {
                isReset = false;
            }
        }

        // Incrementing or resetting number
        StringBuilder incrementedNumberSB = new StringBuilder();
        int incrementedNumberInt;
        if (isReset) {
            for (int i = 0; i < numberToIncrement.length(); i++){
                incrementedNumberSB.append("0");
            }
        } else {
            incrementedNumberInt = Integer.parseInt(numberToIncrement) + 1;
            incrementedNumberSB.append(incrementedNumberInt);
        }

        // Creating result string according to referenceNumber structure
        String incrementedNumberString = padLeftZeros(incrementedNumberSB.toString(), numberToIncrement.length());
        String prefix = referenceNumber.substring(0, indexFirstDigit);
        String suffix = referenceNumber.substring(indexLastDigit + 1, referenceNumber.length());
        String result = "";

        if (prefix.length() > 0) {
            result += prefix;
        }
        result += incrementedNumberString;
        if (suffix.length() > 0) {
            result += suffix;
        }

        return result;

    }

}

TL;DR
private String increment(String input){
    char[] chars = input.toCharArray();

    boolean stopLoop = false;
    for(int i = chars.length - 1; i >= 0; i--){
        char character = chars[i];
        switch (character) {
            case '0', '1', '2', '3', '4', '5', '6', '7', '8' -> {
                chars[i] = (char) (character + 1);
                stopLoop = true;
            }
            case '9' -> chars[i] = '0';
            default -> {
                if(i + 1 < chars.length && chars[i + 1] == '0')
                    stopLoop = true;
            }
        }

        if(stopLoop)
            break;
    }

    return  new String(chars);
}

The quizzes of the coding interviews have some goals:

To check if the knowledge that the candidate claims is true.
To filter candidates when you have too many applications.
To know if you think beyond the obvious solution.
To know how deep you know about a language, or a version of it.
To check if you have some knowledge of data structures, algorithms
and time / space complexity of algorigthms.

Here the obvious solution is to work with Strings. Knowing that String in Java are immutable, you have to do all the stuff splitting and concatenating Strings. My proposal is to convert the String into a char array, work with it, and convert it back to an array.
The cost in time of reading and writting into an array is constant, while making all the work with strings has a higher time cost. In an interview is not so important, but for a backend position it is.
Is the char array the best thing ever? It is not, specially in this example. The good of it, is that you can use it like an integer of one byte space in memory. The bad, is because that behaviour, the editor wont tell you that you are writting 0 instead of '0'. 0 is the value of the NUL Ascii character. The Ascii value of '0' is 48. Well, this problem is designed to know if you write too error prone code.
When you have the char array, you traverse it backwards. To traverse backwards is more error prone than to traverse forwards (IndexOutOfBoundExceptions). So, it is another way to check how many errors do you write without the help of your editor, or without compiling several times.
Because you can use char as a value, it is perfect to use it into a switch statement. The first nine cases (from '0' to '8' character) share the same behaviour and code: Increment to next character and end the edition of the characters.
The case of character '9' requires to replace this character with '0' and check the previous character in the array.
The default case is the trickiest. It ignores the characters that aren't numbers if they appear before the characters that are numbers (remember, it is backwards traversing). When one number appears there are two options: the first option is one of the first nine cases, the other option is the case of a '9' converted into a '0'. So, if you find a non number character followed of a '0' the algorithm finishes.
The last step is to convert the char array back into a String, what you can do easily calling one of the String constructors.
You won't face this kind of problem in real life. You will use regular expresions. It is just to check you knowledge, in this case about Java. If you find a problem which solution is too complicate, try a less obvious one.
I hope it helped you.


Factory that chooses between factories based on XML element name I have an implementation of a factory like so:

public class CommandActionFactory : IFactory<ICommandAction, XElement>
{
    private readonly IDictionary<string, IFactory<ICommandAction, XElement>> commandActionFactories;

    public IDictionary<string, IFactory<ICommandAction, XElement>> CommandActionFactories
    {
        get
        {
            return commandActionFactories;
        }
    }

    public CommandActionFactory(IDictionary<string, IFactory<ICommandAction, XElement>> factories)
    {
        if (factories == null)
        {
            throw new ArgumentNullException("factories");
        }

        this.commandActionFactories = factories;
    }

    public ICommandAction Create(XElement parameter)
    {
        if (parameter == null)
        {
            throw new ArgumentNullException("parameter");
        }

        return CommandActionFactories[parameter.Name.LocalName].Create(parameter);
    }
}

This factory picks between factories of the same type, based on the name of the root XML element passed to it. This means I can have bespoke factories for different XML structures, while still referring to only one top-level factory.

FXCop flags the constructor and public getter with the following warning:

  CA1006    Do not nest generic types in member signatures.

So I tried simplifying the generic type as follows:

public interface IXmlCommandActionFactory : IFactory<ICommandAction, XElement>
{
}

public class CommandActionFactory : IXmlCommandActionFactory
{
    private readonly IDictionary<string, IXmlCommandActionFactory> commandActionFactories;

    public IDictionary<string, IXmlCommandActionFactory> CommandActionFactories
    {
        get
        {
            return commandActionFactories;
        }
    }

    public CommandActionFactory(IDictionary<string, IXmlCommandActionFactory> factories)
    {
        if (factories == null)
        {
            throw new ArgumentNullException("factories");
        }

        this.commandActionFactories = factories;
    }

    public ICommandAction Create(XElement parameter)
    {
        if (parameter == null)
        {
            throw new ArgumentNullException("parameter");
        }

        return CommandActionFactories[parameter.Name.LocalName].Create(parameter);
    }
}

However I'm wondering whether, while it makes the issue go away, the new solution is actually hampering readability by hiding the expected type behind another interface.

Which do you think is more readable? This is intended to be a public API.

You can ignore and suppress this warning in your case. This warning is intended to force you to have Dictionary<TypeWithReasonableName1, TypeWithReasonableName2> instead of Dictionary<Action<int>, List<Enumerable<string>>>, but your code is readable enough and shall not produce confusion.


Save total number of attempts NSDefault I'm very new to Swift and programming in general. I'm creating a quiz application and am trying to create a function that will save the number of times a user has completed a quiz.

It appears to work through testing but I was wondering if there was a more elegant way to write this function:

func plusTestsTaken() {
    var testsTakenSave = NSUserDefaults.standardUserDefaults()

    if testsTakenSave.integerForKey("testsTaken") >= 1 {
        var testsTakenCount = testsTakenSave.valueForKey("testsTaken") as! Int
        testsTakenCount++
        testsTakenSave.setValue(testsTakenCount, forKey: "testsTaken")
        testsTakenSave.synchronize()
        println("Test again \(testsTakenCount)")
    } else {
        var testsTakenCount = 1
        testsTakenSave.setValue(testsTakenCount, forKey: "testsTaken")
        testsTakenSave.synchronize()
        println("Test number \(testsTakenCount)")
    }
}

Calling synchronize is completely unnecessary and is actually a performance bottleneck.  For more information about synchronize, take a look at this Stack Overflow answer.  So we can go ahead and remove that line.

We can also get rid of the println as that's not really doing us much good.  There's not much point to it at all, especially if this is an iOS application.

We can make our logic a little better here too.  Let's avoid grabbing our value out of NSUserDefaults multiple times using an if let.  We can also take advantage of the fact that if the value has never been set for our key, grabbing it with integerForKey will return 0.  So, we can handle all scenarios the same:

let kKEY_TestsTaken = "testsTaken"
let defaults = NSUserDefaults.standardUserDefaults()
let testsTaken = defaults.integerForKey(kKEY_TestsTaken)
defaults.setInteger(testsTaken + 1, forKey: kKEY_TestsTaken)

The only remaining comment I have is that we can make our method name more clear.  Also, there's no reason not to give the user the ability to increment the test count by more than one.

func incrementTestCount(value: Int = 1) {
    let kKEY_TestsTaken = "testsTaken"
    let defaults = NSUserDefaults.standardUserDefaults()
    let testsTaken = defaults.integerForKey(kKEY_TestsTaken)
    defaults.setInteger(testsTaken + value, forKey: kKEY_TestsTaken)
}


Reactive operator for distinct values that must change by amount I would like feedback on this DistinctUntilChangedBy operator for System.Reactive that only emits when the current value is different than the last by a specified amount. Is there a better implementation?

I want to avoid multiple emits when the stream looks like this: 0.80, 0.81, 0.80, 0.81, etc.

Using the operator like this:

stream.DistinctUntilChangedBy(0.01).Subscribe(x => Console.WriteLine(x));

Should output this:

0.80

The operator looks like this and it is also on GitHub as a gist:

using System;
using System.Collections.Generic;
using System.Reactive.Linq;

namespace Gists.Reactive.Linq
{
    public static partial class ObservableExtensions
    {
        ///<summary>
        ///Only emit when the current value is different than the last by the specified amount.
        ///</summary>
        public static IObservable<decimal> DistinctUntilChangedBy(this IObservable<decimal> source, decimal amount)
        {
            return source.DistinctUntilChanged(new DistinctUntilChangedByEqualityComparer(amount));
        }

        private class DistinctUntilChangedByEqualityComparer : IEqualityComparer<decimal>
        {
            private readonly decimal amount;

            public DistinctUntilChangedByEqualityComparer(decimal amount)
            {
                this.amount = amount;
            }

            public bool Equals(decimal x, decimal y)
            {
                var diff = Math.Abs(x - y);
                return diff <= Math.Abs(amount);
            }

            public int GetHashCode(decimal obj)
            {
                throw new NotSupportedException();
            }
        }
    }
}

DistinctUntilChangedBy()
This method is public and therefore should do proper argument validation. Since this method is an extension-method, it could be called like:
ObservableExtensions.DistinctUntilChangedBy(someObservable, someDecimal);

If someObservableis null, your method would throw a NullReferenceException where an ArgumentNullException would be a better fit.
Equals(decimal, decimal)
Instead of returning diff <= Math.Abs(amount) you could set the class variable amount in the constructor directly to the Abs(amount) of the passed argument:
this.amount = Math.Abs(amount)

That could speed it up if the Equals() method is called often.
GetHashCode(decimal)
Only if you are 100% sure that this method won't ever be called should you throw a NotSupportedException. See What's the role of GetHashCode in the IEqualityComparer<T> in .NET?


Python function for 7zip compression I wrote a function in Python to call 7zip to compress a folder. The code works and is able to perform the task, however I do not know if this is the most optimal way. Are there any ways to improve the code, any redundant steps?

def Compress_7z(Initial_path,Container_Name,File_List, compression_level = -1,DAC = False):
 """Compression level = -1 standard, 0-9 compression level
       DAC : Delete After Compression - Remove File after being compressed
    """
    try:
        chdir(Initial_path)
    except:
        return('Path does not exist')

    # write a listfile
    with open('list.txt', 'w') as f:
        for item in File_List:
            f.write("%s\n" % item)

    cmd = ['7z', 'a', Container_Name,'@list.txt']

    if compression_level != -1 and compression_level <= 9:
        cmd.append('-mx{0}'.format(compression_level))
    elif compression_level > 9 or compression_level < -1:
        return("Compression not standard: aborting compression")

    system = subprocess.Popen(cmd, stderr=subprocess.STDOUT, stdout=subprocess.PIPE,shell=True)

    while system.poll() == None:  # .poll() will return a value once it's complete.
        time.sleep(1)
    if DAC:
        for f in File_List:
            remove(f)

    # Clean up from compression
    remove('list.txt')
    return(system.communicate())

Simply reading the code already has a few things which stand out to me
(in general this all goes towards the style described in
this document, which is
standard to follow for Python code):

Compress_7z should start lower-case and probably should also say
compress_7zip.  I'm agreeing that the literal "7" would be
appropriate here, given the format name.
DAC is a bad variable name, as you've to read the docstring to even
begin to understand what this parameter is doing.  A better name might
be, well, delete_files_after perhaps.
The rest of the parameters should also all be lower-case.
Try and apply consistent formatting, or use an IDE or
command-line tool to do it
for you (right now there's no whitespace after some of the commas for
example, also some of the quotes are single and some are double for no
good reason).
The docstring is not very standard, though at least it mentions the
value range of the parameters, which is obviously good.

Next up, actual functionality:

Shelling out to the 7z utility is of course an option, although there
are also pure Python libraries for this as far as I can see.  The reason
I point this out is that for one the command-line utility might not be
installed and e.g. pip or a similar tool for Python might make it
easier to handle the dependency.

Since you've already settled on it though, a few more points about the
current implementation:

chdir in a library function is almost certainly a bad idea.  It's
completely not expected that "compress these files" also means "change
my current working directory".  Instead make sure that the function
works without by specifying file names reliably (that is, using
absolute path names, or by specifying the working directory for the
invoked process only).
Using strings as error values is a big no.  Use exceptions, or if you
absolutely do not want to do that, a properly structured return
value.  Right now you couldn't distinguish the return value of the
tool from the strings "Path does not exist" and that's generally to
be avoided.
If the compression level is a string the list.txt file will not be
cleaned up.  That's why you generally want to handle exceptions
properly, look up how
try, catch and finally work
for that.
Finally it seems like the subprocess.Popen invocation is too much
work.  If you don't have to communicate with the invoked process,
simply run it and wait for the result. time.sleep is an extremely
wasteful way of accomplishing the same.  Instead I think you might
just be able to use subprocess.run with the cwd argument (see
above) and be done with it.  You don't need shell=True either, since
the invocation doesn't use any shell features at all.


Restructuring data received from JSON call I'm making a JSON call and receiving data and inputting the data received into some HTML. However I don't think this is the best way to return the data. It's seems like a bit of a hack as I'm injecting a bunch of HTML in my jQuery. I'm also adding an image inside of my jQuery cause I want it to appear between some of the JSON data that is getting returned. Ideally I'd like my html to loop through and generate this info into the chatInfo div. Anybody have any ideas how to better do this?

$.getJSON("/services/chatInfo.json?user=manual" + mediaTypes + locationTypes, function(data) {

         var items = [];
            $.each( data.media, function( key, val ) {
            items.push( "

" + val.displayName + "
" );
            items.push( "
" + val.hours + "

" );
            });

            $("
").html(items.join("")).appendTo( "#chatInfo" );
    });

A much more maintainable solution would be using templates, like Handlebars. This is to primarily avoid HTML in the JS, which can get messy in the long run:

Assuming your data looks like:

{
  media : [
    {"displayName" : "", "hours" : ""},
    ...
  ]
}

Add this to your page. This will be your template. It will not render since it's inside

Then your JS, short and lean.

// Get the template and compile it to a renderer function;
var template = $('#template-chatinfo');
var templateRenderer = Handlebars.compile(template);

// You might want to cache chatInfo
var chatInfo = $('#chatInfo');

// Pull out the url for easy reading and editing
var url = "/services/chatInfo.json?user=manual" + mediaTypes + locationTypes;

$.getJSON(url, function(data) {
  // Render the data and append
  var renderedData = templateRenderer(data);
  $("
").html(renderedData).appendTo(chatInfo);
});


Looping Strategy: Change behaviour after first iteration I have often encountered the following situation. I want to call a method in a Loop, but want to skip the call at the first run. It do not have to be a method, it also can be some lines of code that I want to skip. Here is one example:

I have a file which looks like that.

headline:headline1
content:lorem ipsum dolor sit amet
content2:lorem ipsum dolor sit amet
content3:lorem ipsum dolor sit amet
headline:headline2
content1:lorem ipsum dolor sit amet
.
.
.

I iterate through the lines and generate following html:

 headline1

 lorem ipsum dolor sit amet

 lorem ipsum dolor sit amet

 lorem ipsum dolor sit amet

 headline2

 lorem ipsum dolor sit amet

  .
  .
  .

My Code looks like that:

private String createList(String file) {
        StringBuilder output = new StringBuilder();
        String[] lines = file.split("\n");

        int headlineCounter = 0;
        for (String line : lines) {
            String[] fields = line.split(":");
            if (fields[0].equals("headline")) {
                if (headlineCounter > 0) {
                    output.append("
");
                }
                output.append("
").append(fields[1]).append("
");
                output.append("
");
                headlineCounter++;
            } else {
                output.append("
").append(fields[1]).append("
");
            }
        }
        output.append("
");
        return output.toString();
    }

The content always is wrapped with an
 tag. When a new headline appears the content of the last headline is at the end and has to be closed with
 except it's the first headline of course, because there is no content before, that has to be closed.

In my example I am using a counter. On this way it works, but there are other and better ways I think. How would you solve this problem? This is just an example there are of course other and better ways to generate html, I just wanted to show you an example to my question.

One elegant way to handle this is to parse the input into a more usable structure before transforming it to HTML. For example, each section could be represented as:

private class Section {
  final String headline;
  final List<String> contents = new ArrayList<String>();

  Section(String headline) {
    this.headline = headline;
  }

  String asHtml() {
    StringBuilder sb = new StringBuilde();

    sb.append("
").append(headline).append("
");
    sb.append("
");
    for (String item : contents) {
      sb.append("
").append(item).append("
");
    }
    sb.append("
");

    return sb.toString();
  }
}

Note how the code clearly shows the structure of the resulting HTML.

The parsing code now has a more focused responsibility: parsing and validating the input. It has nothing to do with the output format any more.

private Iterable
 parseList(String fileContents) {
  List
 sections = new LinkedList
();

  for (String line : fileContents.split("\n")) {
    String[] fields = line.split(":");
    if (fields.length != 2) {
      // throw some exception
    }

    if ("headline".equals(fields[0])) {
      sections.add(new Section(fields[1]));
      continue;
    }

    if (sections.isEmpty()) {
      // throw some error because "content" came before "headline"
    }

    sections.getLast().contents.add(fields[1]);
  }

  return sections;
}

I pointed out some lines with comments where you should do input validation. If you don't do this, you might get surprised by  ArrayIndexOutOfBoundsExceptions if the input string is invalid.

Building an object that represents each section has the advantage that invalid HTML cannot be produces – your current code could emit a
 outside of an
.

Then in your main code, the sections are simply concatenated together:

StringBuilder output = new StringBuilder();
for (Sections section : parseList(...)) {
  output.append(section.asHtml());
}

The advantage of separating the responsibilities of parsing and formatting is that it is now much easier to adapt a single part (e.g. to a new input or output format). My suggestion for the Section class has to be criticized here, because it does not follow the Single Responsibility Principle: it both represents a section, and formats the output. Those should ideally be separated into two different classes, so that the formatting can vary without Section having to change.


My take on implementing the Repository Design Pattern in PHP The past few weeks I've been trying to grasp on nearly any Design Pattern ever invented, but the repository pattern is one I could understand and implement it myself in my project. My application uses the MVC pattern where I've got the "V" and the "C" part covered in a very basic way, and I want to get rid of a single model "M" doing everything the view needs. I would like to know if I'm somewhat in the right direction. Along with that I ask some questions on parts of the approach.

Let's get started with some code, the whole operation is to get the latest log events from an external data source (database):

Controller

My controller creates a new Repository object, which takes the Database wrapper and the Data Mapper as its arguments. The Data Mapper takes a Domain Entity.

How would I take care of these 4 require_once statements in my Controller functions?

public function index() {
    // need a way to not have to require these classes all the time
    require_once '../app/domain/repositories/EventRepository.php';
    require_once '../app/domain/interfaces/EventRepositoryInterface.php';
    require_once '../app/domain/mappers/EventMapper.php';
    require_once '../app/domain/entities/Event.php';

    $events = new EventRepository(new Database('db'), new EventMapper(new Event()));

    try {
        $return = $events->fetchAll();
    } catch (DatabaseException $e) {
        $return = $e->getErrorMessage();
    }

    $this->view('/home/dashboard', ['return' => $return]);
}

Repository

class EventRepository implements EventRepositoryInterface {

    protected $dbh;
    protected $eventMapper;

    public function __construct(Database $dbh, EventMapper $eventMapper) {
        $this->dbh = $dbh;
        $this->eventMapper = $eventMapper;
    }

    public function fetchAll() {
        $data = $this->dbh->sql_select('SELECT datetime, message FROM eventlog');

        return $this->eventMapper->toEvent($data);
    }

}

Repository Interface

interface EventRepositoryInterface {
    public function fetchAll();
}

Data Mapper

I'm using a Data Mapper in my Repository Pattern approach. Not sure if this is a right way to do it, but I've seen examples of implementing Data Mappers with repositories.

class EventMapper {

    protected $collection = [];
    protected $event;

    public function __construct(Event $event) {
        $this->event = $event;
    }

    public function toEvent($data) {
        foreach($data as $value) {
            $this->event->setDateTime($value['datetime']);
            $this->event->setMessage($value['message']);

            $this->collection[] = $this->event;
        }

        return $this->collection;
    }
}

Domain Entity

class Event {

    protected $id;
    protected $dateTime;
    protected $message;
    protected $type;

    public function setDateTime($dateTime) {
         $this->dateTime = $dateTime;
    }

    public function getDateTime() {
         return $this->dateTime;
    }

    public function setMessage($message) {
        $this->message = $message;
    }

    public function getMessage() {
        return $this->message;
    }

}

This code leaves me with an array of objects which I can do other stuff with it. Eventually the data from this array needs to be converted to an HTML table. Where would I put such a class (like TablePrinter.php) in my application?

The autoloading problem.
Using composer is probably the easiest way to autoload classes in PHP right now. You'll only have to declare namespaces, use them, and everything will work with only including the autoload file from composer once.
About your repository pattern implementation.
"Repository Interface"
Nope. Not true. You created an EventRepositoryInterface. The point of an interface is to expose an interface that can be shared by classes implementing it, to be able to modify them (or create other implementations) without having to change the code using them in the first place. Are you going to create a new EventRepository class ?
I'm pretty sure you actually wanted a RepositoryInterface that EventRepository will implement (as well as your other repositories).
EventRepository
This class should be able to handle the $data to make Events one by one, thanks to EventMapper.
Right now its only use is to avoid duplicating SQL requests through the code.
It should check the $data, loop over it, do whatever is needed to get a standard data format (such as an array). Then it should loop over this formatted data, pass each iteration to EventMapper and return an array of Events.
EventMapper
As said earlier, this class should only receive data for one Event and return only one aswell. Also, even though you should have sanitized $data beforehand, you still should check it in this class too. To end with a little nitpicking there, toEvent($data) is usually called hydrate($data).


Creation of MxM matrix: Python version much slower than Matlab version I have written the same code in Python (NumPy) and in Matlab, and I tried to use the same structure for both language and follow the same procedure. Now my problem is that when I run my code in Python it's very very slow but it runs fast in Matlab.

How can I improve my Python code?  I want to work on the Python version of my code.

In the code I am trying to create an MxM matrix (M can vary between 100 to 1000) and trying to fill this matrix 16 times, each time I add the previous value of M[i,j] with the current value. The operation inside the code is simple just some add and divide, and now I wonder why Matlab performs better than Python.

Python:

import numpy as np
import matplotlib.pyplot as plt

from datetime import datetime
startTime = datetime.now()

bb = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]
bb = ['{0:04b}'.format(x) for x in bb]

step_prob=0.01

step = int(1/step_prob)

out_t = np.zeros((step,step), dtype=float)
out_f = np.zeros((step,step), dtype=float)
s = np.zeros((step,step), dtype=float)
improve = np.empty((step,step))

for ii in range(1, step+1):
    print ii
    pti=ii*step_prob
    for  jj in range(1, step+1):
        pfi=jj*step_prob
        improve[ii-1,jj-1]=0
        si=(1-pfi+pti)/2
        for k in range(len(bb)):
            f1 = int(bb[k][0])
            f2 = int(bb[k][1])
            f3 = int(bb[k][2])
            f4 = int(bb[k][3])
            for i in range(1, step+1):
                for j in range(1, step+1):
                    ptj=i*step_prob
                    pfj=j*step_prob
                    out_t[i-1,j-1] = f1*pti*ptj+f2*pti*(1-ptj)+f3*(1-pti)*ptj+f4*(1-pti)*(1-ptj)
                    out_f[i-1,j-1] = f1*pfi*pfj+f2*pfi*(1-pfj)+f3*(1-pfi)*pfj+f4*(1-pfi)*(1-pfj)
                    sj=(1-pfj+ptj)/2;
                    s[i-1,j-1] = ((1-out_f[i-1,j-1]+out_t[i-1,j-1])/2)-max(si,sj);

#         temp=s*(s>0)*tril(ones(size(s)));
            temp = s*(s>0)*np.tril(np.ones((len(s),len(s))).astype(int))
            improve[ii-1,jj-1]=improve[ii-1,jj-1]+sum(sum(temp))

im = plt.imshow(improve*np.tril(np.ones((len(improve),len(improve))).astype(int)),interpolation='bilinear', aspect='auto') # s*[s>0]
# im = plt.imshow(improve, interpolation='bilinear', aspect='auto') # s*[s>0]
plt.colorbar(im, orientation='vertical')
plt.show()

Matlab:

close all
clear all
clc

bb = de2bi([0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15],4,'left-msb');
% bb = de2bi([1],4);

step_prob=0.01;
for ii=1:1*(1/step_prob)
    ii
    pti=ii*step_prob;
    for jj=1:1*(1/step_prob)
        pfi=jj*step_prob;
        improve(ii,jj)=0;
        si=(1-pfi+pti)/2;
        for k=1:size(bb,1)
            f1=bb(k,1);
            f2=bb(k,2);
            f3=bb(k,3);
            f4=bb(k,4);
            for i=1:1*(1/step_prob)
                for j=1:1*(1/step_prob)
                    ptj=i*step_prob;
                    pfj=j*step_prob;
                    out_t(i,j)=f1*pti*ptj+f2*pti*(1-ptj)+f3*(1-pti)*ptj+f4*(1-pti)*(1-ptj);
                    out_f(i,j)=f1*pfi*pfj+f2*pfi*(1-pfj)+f3*(1-pfi)*pfj+f4*(1-pfi)*(1-pfj);
                    sj=(1-pfj+ptj)/2;
                    s(i,j)=((1-out_f(i,j)+out_t(i,j))/2)-max(si,sj);
                end
            end
            temp=s.*(s>0).*tril(ones(size(s)));
            improve(ii,jj)=improve(ii,jj)+sum(sum(temp));
            clear f1 f2 f3 f4
        end
    end
end
scale=step_prob:step_prob:1;
imagesc(scale,scale,improve.*tril(ones(size(improve))))
xlabel('Pfj')
ylabel('Ptj')
axis xy

You use very short variable names, so its hard to follow what your code is doing.

For speed in numpy, you need to use vector operations. So instead of writing

for ii in range(step):
    out[ii] = ii * x + b

You should write:

out = numpy.arange(step)*x + b

Individually accessing the elements of numpy array is actually quite slow. You need to write your code to do everything in vector operations.

Here is my rewrite of your inner loop to be vectorized. You could vectorize the outer loop as well, but this should make the biggest difference.

        f1 = int(bb[k][0])
        f2 = int(bb[k][1])
        f3 = int(bb[k][2])
        f4 = int(bb[k][3])

        ptj = np.arange(step_prob, 1.0 + step_prob, step_prob)[:, None]
        pfj = np.arange(step_prob, 1.0 + step_prob, step_prob)[None, :]

        out_t = f1*pti*ptj+f2*pti*(1-ptj)+f3*(1-pti)*ptj+f4*(1-pti)*(1-ptj)
        out_f = f1*pfi*pfj+f2*pfi*(1-pfj)+f3*(1-pfi)*pfj+f4*(1-pfi)*(1-pfj)
        sj = (1 - pfj + ptj) / 2

        the_max = np.maximum(sj, [si])
        s = ((1-out_f+out_t)/2)-the_max

        temp = s*(s>0)*np.tril(np.ones((len(s),len(s))).astype(int))
        improve[ii-1,jj-1]=improve[ii-1,jj-1]+temp.sum()


Change-making problem with specific constraints I am a new Pythoner.  I have solved a OJ project with Python. Can you tell me how can I solve it more Pythonically?

Problem Description: There are n coin denominations, each with an unlimited supply of coins. Use them to make a given total amount m.
Requirements:

Every denomination must be used
The number of coins of small denominations must be greater than the number of large denomination coins
The total number of coins must be minimum
If there is no combination of conditions to meet the conditions 1 and 2, the output "NA"

Input parameters:

coin types n: 2 < n < 10
The total amount m:  m < 1000
The largest denomination is less than 20

Program input: First line: the total amount requested Second row: each
coin denomination (in half-space separated)
Program output: First line: total number of coins Second row: the
number of small to large denomination of each coin (in half-space
separated)
Sample input:
20
1 2 5

Sample output:
9
4 3 2

#!/bin/bash/env python3
import copy

def coinChange():
    amount, coins = int(input()), list(sorted(map(int, input().split(sep=' '))))
    min_package = [x for x in range(len(coins), 0, -1)]
    min_sum = sum(map(lambda x, y:x * y, min_package , coins))

    if amount < min_sum:
        print('NA')
        return

    dp, dp[0] = [amount + 1] * (amount + 1), 0
    packages = [[0] * len(coins)] * (len(dp))
    for i in range(len(dp)):
        for j, coin in enumerate (coins):
            if coin <= i:
                last = i - coin
                package = copy.deepcopy(packages[last])
                if dp[i] > dp[last] + 1 and (j == 0 or package[j - 1] > package[j] + 1):
                    dp[i], package[j], packages[i] = dp[last] + 1, package[j] + 1, package
            else:
                break

    if dp[-1] > amount or packages[-1][-1] < 1:
        print('NA')
    else:
        print(sum(packages[-1]))
        print(' '.join([str(x) for x in packages[-1]]))

if __name__ == '__main__':
    coinChange()

1. Review

The function coinChange carries out three tasks: (i) read the input; (ii) solve the problem; (iii) print the solution. This makes it hard to test, because it's hard to run the code on different problem instances (you have to feed it the problem in text form on standard input), and becaue it's hard to automatically check the output (you have to capture the standard output).

It would be easier to test the code if task (ii) were refactored into a separate function. I'd write something like this:

class NoSolution(Exception):
    pass

def coin_change(amount, coins):
    """Given a target amount and a sorted list of coins denominations,
    determine how to make change for amount with the minimum number of
    coins, subject to the restrictions that every denomination is used
    and the number of coins of a small denomination must be greater
    than the number of coins of a large denomination. Return a list
    giving the number of coins of each denomination, or raise
    NoSolution if making change is impossible under the constraints.

    """
    # ... see below ...

def main():
    """Read coin change problem from standard input and write solution to
    standard output, or NA if there is no solution.

    """
    amount, coins = int(input()), list(sorted(map(int, input().split())))
    try:
        result = coin_change(amount, coins)
    except NoSolution:
        print('NA')
    else:
        print(sum(result))
        print(*result)

and now it's easy to test the code, for example from the interactive interpreter:

>>> coin_change(20, [1, 2, 5])
[4, 3, 2]

sorted already returns a list, so there is no need to pass the result to list.
str.split splits on whitespace by default, so there is no need to pass sep=' ' in this case.
Note in the code above that I handle an exceptional result by raising an exception (instead of returning some exceptional value). This is more robust because it is easy to forget to check the result of a function, causing exceptional cases to be silently mishandled.
These lines are unnecessary:

min_package = [x for x in range(len(coins), 0, -1)]
min_sum = sum(map(lambda x, y:x * y, min_package , coins))

if amount < min_sum:
    print('NA')
    return

That's because there is already logic for detecting unsolvable problem instances later in the function. There's no point in doing this twice: duplicate code has twice the opportunties for bugs.
The line:

dp, dp[0] = [amount + 1] * (amount + 1), 0

is kind of tricksy (the reader would need to be very sure of Python's order-of-evaluation rules to know that dp[0] is evaluated after dp is assigned), and so it would be simpler to write something like:

dp = [amount + 1] * (amount + 1)
dp[0] = 0

or alternatively,

dp = [0] + [amount + 1] * amount

The only role of amount + 1 in this line is to be greater than every possible number of coins. I think it would be clearer to use a value like float('inf') here.
This loop:

for i in range(len(dp)):

would be better written like this:

for i in range(1, amount + 1):

This is because there's no point in considering making change for 0, so we might as well start at 1, and because amount + 1 makes it clear that we consider all values up to amount, without having to remember how many elements there are in the list dp.
In Python we prefer not to use tuple assignment just to save on lines of code (only when we really do have a tuple to assign). This kind of code is hard to follow:

dp[i], package[j], packages[i] = dp[last] + 1, package[j] + 1, package

because you have to count carefully to see which expression on the right is assigned to which expression on the left. I think it's clearer to write:

dp[i] = dp[last] + 1
package[j] += 1
packages[i] = package

Notice that I can use the += operator here, which wasn't possible in the tuple assignment version of the code.
Making a deep copy of packages[last] is unecessary: this is a list of integers, and so a shallow copy would work.
There's no need to import the copy module just to take a shallow copy of a list. It's simpler to use the built-in function list.
The code takes a copy of packages[last] before testing whether coin could be used. But this is a waste if coin in fact cannot be used. It would be better to take the copy after the test, like this:

package = packages[last]
if dp[i] > dp[last] + 1 and (j == 0 or package[j - 1] > package[j] + 1):
    dp[i] = dp[last] + 1
    new_package = list(package)
    new_package[j] += 1
    packages[i] = new_package

The meaning of the name dp is not clear to me. The value dp[i] is the smallest number of coins found so far that makes change for i under the restrictions of the problem. Even knowing this, I can't work out what "dp" stands for! I would use a name like min_coins together with a comment explaining the meaning.

Update: does "dp" stand for "dynamic programming"??
The meaning of the name package is also not clear. The value packages[i][j] is the number of coins of the j'th denomination in the best solution found so far that makes change for i under the restrictions of the problem. So I'd use a name like solutions together with a comment.
Instead of:

    if coin <= i:
        # ...
    else:
        break

I would write:

    if coin > i:
        break
    # ...

which saves a level of indentation. In general, it's a good idea to dispose of exceptional cases first.
At the end of the function, the code uses packages[-1] to get the solution for amount. It would be clearer to write packages[amount]. Then the reader wouldn't have to remember how many elements there are in the list packages.
In the line:

if dp[-1] > amount or packages[-1][-1] < 1:

the first part of the condition (dp[-1] > amount) is unnecessary; the second part of the condition suffices.

2. Revised code

def coin_change(amount, coins):
    """Given a target amount and a sorted list of coins denominations,
    determine how to make change for amount with the minimum number of
    coins, subject to the restrictions that every denomination is used
    and the number of coins of a small denomination must be greater
    than the number of coins of a large denomination. Return a list
    giving the number of coins of each denomination, or raise
    NoSolution if making change is impossible under the constraints.

    """
    # min_coins[i] is smallest number of coins found making change for i.
    min_coins = [0] + [float('inf')] * amount

    # solutions[i][j] is the number of coins of the j'th denomination in
    # the best solution found making change for i.
    solutions = [[0] * len(coins)] * (amount + 1)

    for i in range(1, amount + 1):
        for j, coin in enumerate(coins):
            if coin > i:
                break
            last = i - coin
            solution = solutions[last]
            if (min_coins[i] > min_coins[last] + 1
                and (j == 0 or solution[j - 1] > solution[j] + 1)):
                min_coins[i] = min_coins[last] + 1
                new_solution = list(solution)
                new_solution[j] += 1
                solutions[i] = new_solution

    if solutions[amount][-1] == 0:
        raise NoSolution()
    else:
        return solutions[amount]

3. Improved algorithm

The data structure packages ends up containing len(coins) * (amount + 1) values. But this is a waste of memory and computation, because you ought to be able to describe each solution by giving only the denomination of the highest coin used in that solution. Then you could subtract that coin and look up the remainder to get the next coin, and so on.

But if you try to implement this then you'll struggle, because in the revised data structure you can't apply the restriction that every denomination is used and the number of coins of a small denomination must be greater than the number of coins of a large denomination.

However, there's a way to transform the problem so that it turns into an ordinary (unrestricted) change-making problem. The idea is that if you're given the problem instance:

  make change for 20 using fewest coins with values 1, 2, 5 (with the restriction)

then you transform it into:

  make change for 8 using fewest coins with values 1, 3, 8 (without the restriction)

which has the solution 0, 0, 1 which transforms into the solution 4, 3, 2 in the original problem.

No doubt you're asking, what is this transformation and how does it work? There are two steps in the transformation:

Any solution has to contain at least one of the highest denomination of coin (because all denominations have to be included), at least two of the second highest denomination (because the number of coins of a small denomination must be greater than the number of coins of a large denomination), at least three of the third highest denomination, and so on. So we can start by subtracting all these coins from the amount, and then drop the restriction that all denominations have to be included.

In the example above, we know that the solution has to include at least 3×1, 2×2 and 1×5, so we start by subtracting 12 from the amount, getting 8.
Now, whenever we add a coin of some denomination to the solution we're building up, we have to add coins of all smaller denominations too, to maintain the restriction that number of coins of a small denomination must be greater than the number of coins of a large denomination. So if we add a coin of denomination 2, then we have to add a coin of denomination 1 too. And if we add a 5, we have to add 2 and 1 too.

But if a 2 always has to be accompanied by a 1, this is almost the same as having a coin of denomination 3. And if 5 always has to be accompanied by a 2 and a 1, this is almost the same as having a coin of denomination 8. So we can transform the problem into one where the coins have values 1, 3, and 8, and then drop the restriction that number of coins of a small denomination must be greater than the number of coins of a large denomination.

I wrote "almost the same" above because there is a difference in how we have to count the number of coins in order to ensure we meet the "minimum number of coins" condition. See the line marked "(A)" in the code below.

In the transformed problem we can represent the solutions more compactly as discussed above, and then reconstruct the full solution coin by coin at the end. Here's an implementation.

from itertools import accumulate

def coin_change(amount, coins):
    """Given a target amount and a sorted list of coins denominations,
    determine how to make change for amount with the minimum number of
    coins, subject to the restrictions that every denomination is used
    and the number of coins of a small denomination must be greater
    than the number of coins of a large denomination. Return a list
    giving the number of coins of each denomination, or raise
    NoSolution if making change is impossible under the constraints.

    """
    # Transform problem into (almost) standard change-making problem.
    coins = list(accumulate(coins))                    # see (2) above
    amount -= sum(coins)                               # see (1) above
    if amount < 0:
        raise NoSolution()

    # min_coins[i] is smallest number of coins found making change for i.
    min_coins = [0] + [float('inf')] * amount

    # solutions[i] is the index of the highest coin in the best
    # solution found making change for i, or -1 if no solution has
    # been found.
    solutions = [-1] * (amount + 1)

    # Solve the transformed problem
    for i in range(1, amount + 1):
        for j, coin in enumerate(coins):
            if coin > i:
                break
            last = i - coin
            new_coins = min_coins[last] + j + 1        # (A)
            if min_coins[i] > new_coins:
                min_coins[i] = new_coins
                solutions[i] = j

    # Reconstruct solution in the transformed problem.
    result = [0] * len(coins)
    while True:
        j = solutions[amount]
        if j == -1:
            break
        result[j] += 1
        amount -= coins[j]
    if amount != 0:
        raise NoSolution()

    # Transform solution back again.
    return list(accumulate(r + 1 for r in reversed(result)))[::-1]


Decoding data from a MIDI controller as eight buttons I thought about using loops but I don't know how I would do this. if I used for loops would it work the same way as the code down below?

if data[-8]=="B" and data[-5]=="1" and data[-4]=="6":
    print("Button1")
if data[-8]=="B" and data[-5]=="1" and data[-4]=="7":
    print("Button2")
if data[-8]=="B" and data[-5]=="1" and data[-4]=="8":
    print("Button3")
if data[-8]=="B" and data[-5]=="1" and data[-4]=="9":
    print("Button4")
if data[-8]=="B" and data[-5]=="1" and data[-4]=="A":
    print("Button5")
if data[-8]=="B" and data[-5]=="1" and data[-4]=="B":
    print("Button6")
if data[-8]=="B" and data[-5]=="1" and data[-4]=="C":
    print("Button7")
if data[-8]=="B" and data[-5]=="1" and data[-4]=="D":
    print("Button8")

EDIT: When I press a button it prints out the number ex. If I press button one it prints button one. And it would work the same for other at and given time. It'll be on a while true loop.

Use a dict for lookups
When doing lookups, Python provides an associative array data structure called a dict.  Your stacked ifs can be cleanly written as a dict lookup like:
button_text = {
    ("B", "1", "6"): "Button1",
    ("B", "1", "7"): "Button2",
    ("B", "1", "8"): "Button3",
    ("B", "1", "9"): "Button4",
    ("B", "1", "A"): "Button5",
    ("B", "1", "B"): "Button6",
    ("B", "1", "C"): "Button7",
    ("B", "1", "D"): "Button8",
}

print(button_text[data[-8], data[-5], data[-4]])


Iterate over a short list, pairing the elements with the sequential elements of a longer list I am a relatively new programmer. I am trying to use iter and cycle to cycle over a list (key) and 'pair' each item with each consecutive item in the second list (message). The code below works as expected, producing the desired pairs and stopping when it is done. It does not, however, execute that last print statement ('Okay, it worked!') because it hits 'StopIteration' instead.

I can see some kludgy ways to get around this, but how can I more neatly "handle" the StopIteration and have the code "continue"?

key = [3, 1, 20]
message = ['a', 'b', 'c', 'd', 'e', 'f']

print(key)
print(message)
print('\n')

# Try making a for loop that cycles through the key and iterates
# over the message
m = iter(message)
for k in cycle(key):
    print(f'{k}:{next(m)}')
print('Okay, it worked!')
```

Creating a "manual" iterator using iter and then calling next can be tricky, as you found out.
Once an iterator is exhausted, it raises a StopIteration to signal its end.
If you do not catch that in a try/except block, your program will exit ungracefully.

Luckily, message, being a list, is perfectly iterable without being explicit about it.
Simply using it in a for loop will do the iter/next magic for you, and the loop will exit once StopIteration is reached, without error.

zip is a built-in so you can iterate over multiple iterables at once.
zip will iterate until the shortest of the passed iterables is exhausted.
This can be tricky if you expect the iterables to be of the same length.
There is currently talk about a strict flag in the Python ideas mailing list, which would throw errors if strict=True and the iterables are of different lengths.

Assuming you meant to include from itertools import cycle, alongside cycle, of course message is exhausted first.
cycle being infinite, this suprises no one in this case.
The for loop then simply ends after the last message element.

This hopefully does what you want to do:

from itertools import cycle

keys = [3, 1, 20]
message = ["a", "b", "c", "d", "e", "f"]

print(keys, message, sep="\n", end="\n" * 2)

for key, letter in zip(cycle(keys), message):
    print(f"{key}: {letter}")

print("Okay, it worked!")

This is more like your version, needlessly verbose.
Never do this in reality!
However, it can help you understand what is going on in your case:

from itertools import cycle

keys = [3, 1, 20]
message = ["a", "b", "c", "d", "e", "f"]

print(keys, message, sep="\n", end="\n" * 2)

message_iter = iter(message)

for key in cycle(keys):
    try:
        print(f"{key}: {next(message_iter)}")
    except StopIteration:
        break  # Break out of infinite loop

print("Okay, it worked!")


Pascal's Triangle without using combinations I wrote this code for Pascal's Triangle using recursion. Can someone please help me to review it for better performance?

# argument count is number of rows which is entered from terminal.
def pascal_t(count,input_list=0):

    if not count: exit() # if count is 0 program will exit.

    if not input_list: # to print first element as 1 when there is no input list.
        created_array=[1]
        print (count*"   "),"    ".join(map(str,created_array))

        pascal_t((count-1),created_array) # recursive call; created array will be taken as input list.

    created_array=[] # initializing list

    # for loop to insert numbers in created_array
    for index in range(len(input_list)):
        if not index: # when index is 0 then first value will be inserted in created_ array
            if (index+1)==len(input_list): # if there is only 1 element in the list, this condition will insert first element again in created_array
                created_array.append(input_list[index])
            created_array.append(input_list[index])

        elif (index+1)==len(input_list):    # when index is second last, it will insert two elements in created_array
            created_array.extend((input_list[index-1]+input_list[index],input_list[index]))

        else:
            created_array.append(input_list[index-1]+input_list[index])

    # list formatting for proper pattern printing.
    formatted_list = [str(num)+"   " if num>9 and num<=99 else str(num)+"  " if num>99 and num<=999 else str(num)+" " if num>999 and num<=9999 else str(num)+"" if num>9999 else str(num)+"    " for num in created_array]

    # Creating and printing string from created_array
    string_to_print = " ".join(formatted_list)
    print count*"   ",string_to_print
    pascal_t((count-1),created_array) # recursive call; created array will be taken as input list.

# function call with user input from terminal
pascal_t(int(raw_input("Enter Number of Rows(1-20) for Pascal's Triangle: ")))

Someone already commented about not using recursion but if you still want to keep it, here are my comments.

There are quite a few style issues, if you install the pep8 command and run from the command line pep8 pascal.py (or whatever the filename is), you'll see them (multiple statements on one line, missing whitespace around operator, line too long).

Other than that, a few things:

I'd definitely check that the input is a number, you don't want to feed letters or other characters to your function.
The input_list parameter should be initialized properly. It's supposed to be a list, not an integer. But since it's not nice to initialize it to [], you can set it to None and check on that. (For an explanation why this is not nice, you can have a quick look here)
Your function name is pascal_t, to me that looks like it returns a pascal triangle, while it actually prints it. I'd say either rename it to print_pascal_t or make it return an object and print that.
A lot of comments. You can remove quite a few of them just by giving proper names. For example, count can be renamed to rows_number and you can remove the first line. You can be more explicit with the first check (if rows_number <= 0: ) and remove that comment, too. The other comments are simply describing what happens. I think good comment should be describing why things happen, otherwise it's either useless, or you need to write code more clearly.
The first check on if not index is useless, you can remove that part of code.
You also don't need to create an array with a single element so that you can map, cast and join it.
You exit() the program inside of your function instead of just returning. I'd check if input_list is None and just return in that case.
The formatting line can be converted to something much simpler using, well, format :-)
Just as a side note, this will not work properly in python3.

Modified code:

def pascal_t(rows_number, input_list = None):
    if rows_number <= 0:
        return

    if not input_list:
        print(rows_number * "   "), "1"
        pascal_t((rows_number - 1), [1])
        return

    created_array = []

    for index in range(len(input_list)):
        if (index + 1) == len(input_list):
            created_array.extend((input_list[index - 1] + input_list[index], input_list[index]))
        else:
            created_array.append(input_list[index - 1] + input_list[index])

    formatted_list = ['{0: <5}'.format(str(num)) for num in created_array]

    print rows_number * "   ", " ".join(formatted_list)
    pascal_t((rows_number - 1), created_array)

user_input = raw_input("Enter Number of Rows(1-20) for Pascal's Triangle: ")
try:
    rows_number = int(user_input)
except ValueError:
    print "Not a valid number"
else:
    pascal_t(rows_number)


Tic-tac-toe 'get winner' algorithm As a first step to attempting the Weekend Challenger Reboot, I decided to first implement a regular 'tic-tac-toe' game and then expand it later to be 'ultimate tic-tac-toe'.

Below is the code for the tic-tac-toe board. It's fully functional and unit tested, but I'm not too happy about the getWinner() method. This method's purpose is to check to see if there is any winner in the game, and to return the identity of said winner if they exist.

The reasons I don't like it are:

The method is very long, which immediately sets off warning flags in my mind.
It makes use of a 'flag' type variable that tracks if we've found a winner yet. I don't like flag variables because they're really something that I got in the habit of doing back when I did procedural programming, and therefore probably shouldn't exist in an object-oriented design.
There's an awful lot of continue and break statements which seem to indicate poorly-designed loops.
I'm pretty sure that if there is no winner, this method is O(n2).

This is the code for the getWinner() method:

public class Board {
    private final Player[][] fields;
    ...
    public Player getWinner() {
        Player winner = null;
        boolean isWon = false;

        // check columns (same x)
        for (int x = 0; x < fields.length; x++) {
            Player value = fields[x][0];

            if (value == null) {
                continue;
            }
            for (int y = 1; y < fields[x].length; y++) {
                Player current = fields[x][y];
                if (current == null || !current.equals(value)) {
                    break;
                }
                if (y == fields[x].length -1) {
                    isWon = true;
                    winner = value;
                }
            }
            if(isWon) {
                break;
            }
        }

        if (! isWon) {
            // check rows (same y)

            for (int y = 0; y < fields[0].length; y++) {
                Player value = fields[0][y];
                if (value == null) {
                    continue;
                }
                for (int x = 1; x < fields.length; x++) {
                    Player current = fields[x][y];
                    if (current == null || !current.equals(value)) {
                        break;
                    }
                    if (x == fields.length -1) {
                        isWon = true;
                        winner = value;
                    }
                }
                if(isWon) {
                    break;
                }
            }

        }
        if (! isWon) {
            // check diagonal (bottom left to top right

            Player value = fields[0][0];
            if (value != null) {
                for (int i = 1; i < fields.length; i++) {
                    if (fields[i][i] != value) {
                        break;
                    }
                    if (i == fields.length -1) {
                        isWon = true;
                        winner = value;
                    }
                }
            }
        }

        if (! isWon) {
            // check anti-diagonal (top left to bottom right)
            int length = fields.length;
            Player value = fields[0][length-1];
            if (value != null) {
                for (int i = 1; i < length; i++) {
                    if (fields[i][length-i-1] != value) {
                        break;
                    }
                    if (i == length -1) {
                        isWon = true;
                        winner = value;
                    }
                }
            }
        }
        return winner;
    }
    ...
}

The rest of the code is mostly irrelevant. Something important to be aware of is that the board will always be a perfect square -- eg. 3x3 or 4x4 or 5x5. The Player is a simple enum:

public enum Player {
    X("X"),
    O("O");

    private String str;
    private Player(String str) {
        this.str = str;
    }

    @Override public String toString() {
        return str;
    }
}

It's method extraction time!

But before that, I have to comment on your Player enum:

str is a bad name, name would be better.
Speaking of name, all enums have a name() method by default. You don't need your str variable, return name(); instead.
Speaking of return name();, that's exactly what the default implementation of toString() already does for enums. Absolutely no need to override it.

And therefore, we've reduced your Player enum to:

public enum Player {
    X, O;
}

Ain't it lovely with enums? :)

Now, back to your getWinner() method:

You have a whole bunch of duplicated code there indeed. It would be handy if you could get a Collection of some kind (or an array), add some elements to it and check: Is there a winner given by these Player values?

This is just one version of doing it, it's not the optimal one but it should get you started. This code will add a bunch of Player objects to a list and then we check if those objects match to find if there's a winner.

List<Player> players = new ArrayList<>();
for (int x = 0; x < fields.length; x++) {
    for (int y = 0; y < fields[x].length; y++) {
        players.add(fields[x][y]);
    }
    Player winner = findWinner(players);
    if (winner != null)
        return winner;
}

Player findWinner(List<Player> players) {
    Player win = players.get(0);
    for (Player pl : players) {
        // it's fine that we loop through index 0 again,
        // even though we've already processed it.
        // It's a fast operation and it won't change the result
        if (pl != win)
            return null;
    }
    return pl;
}

Please note that there are even more improvements possible for the getWinner method, I don't want to reveal all my secrets for now though ;) And also, this is just one way of doing it, which will reduce your code duplication a bit at least. There are other possible approaches here as well.


Minimum swaps algorithm terminated due to timeout I have been trying to solve this question.

  Given an unordered array consisting of consecutive integers [1, 2, 3, …, n], find the minimum number of two-element swaps to sort the array.

I was able to solve it but my solution's complexity is not good enough that it terminated due to a timeout when it runs with bigger arrays. This is a typical problem for me, I always solve the problem somehow but the complexity is not optimal and the solution does not pass the all the cast cases. If you have any suggestions for me for the future interview questions like that, I'd appreciate knowing how should I approach the questions.

function findIndice(arr,i){
  let iterator=0
  while(arr[iterator]!==i+1){
    iterator++
  }
  return iterator
}

function swap(arr,x,y){
  let temp=arr[x]
  arr[x]=arr[y]
  arr[y]=temp
}

function minimumSwaps(arr){
  let i=0;
  let counter=0;
  let size=arr.length;

  for(i=0;i<size-1;i++){
    if(arr[i]!==i+1){
      let index=findIndice(arr,i)
      swap(arr,index,i)
      counter++
    }
  }
  return counter
}

Your algorithm can be summarized by the pseudocode:

for each position in the array
    if a position is occupied by the wrong number
        find the number that fits into the position
        perform a swap

A better algorithm would be:

for each position in the array
    if a number is in the wrong location
        find the position the number should go
        perform a swap

This is because finding a number for a given location requires a linear scan, but finding the location for a given number is as simple as it gets: the number five should go into the fifth position.

The full program could look like this (in python, as I don't speak js):

def minimumSwaps(arr):
    def swap(i, j):
        arr[i], arr[j] = arr[j], arr[i]
    swaps = 0
    for i in range(0, len(arr)):
        while arr[i]-1 != i:
            swap(i, arr[i]-1)
            swaps += 1
    return swaps


Byte conversion in my Caesar's cipher I was wondering if I am doing any useless conversions byte←→int, for example:

byte((int(ch-'A')+shift)%26 + 'A')

Converting ch-'A' to an int is because the shift argument can be negative (to implement the decode function). I couldn't figure out a simpler way to negate the operation using bytes.

package main

import (
    "bufio"
    "flag"
    "fmt"
    "os"
)

func main() {
    shift := flag.Int("shift", 13, "Cipher shift")
    decode := flag.Bool("decode", false, "Decode input")

    flag.Parse()

    scanner := bufio.NewScanner(os.Stdin)
    for scanner.Scan() {
        if *decode {
            fmt.Println(Decode(scanner.Text(), *shift))
        } else {
            fmt.Println(Encode(scanner.Text(), *shift))
        }
    }
}

func Encode(s string, shift int) string {
    return cipher(s, shift)
}

func Decode(s string, shift int) string {
    return cipher(s, -shift+26)
}

func cipher(s string, shift int) string {
    var line string
    for _, ch := range []byte(s) {
        if ch >= 'A' && ch <= 'Z' {
            ch = byte((int(ch-'A')+shift)%26 + 'A')
        } else if ch >= 'a' && ch <= 'z' {
            ch = byte((int(ch-'a')+shift)%26 + 'a')
        }
        line += string(ch)
    }
    return line
}

Flags

It's not very problematic for very short programs like this, but better get good habits: flags should be outside of all code blocks; so that you detect immediately if you have a flag naming conflict between different files.

var (
    shiftF = flag.Int("shift", 13, "Cipher shift")
    decodeF = flag.Bool("decode", false, "Decode input")
)

func main() {
    …
}

Code organization

You're checking for the value of the -decode flag every time you scan a line, and recomputing -shift+26 every time as well: not very efficient, nor super readable. As you noticed, the only difference between encoding and decoding is the value of shift. Why not do something like:

func main() {
    flag.Parse()

    // we could also directly change the value of the flag, but I find it
    // less readable — it's better to treat flag values as immutable.
    var shift int
    if *decode {
        shift = 26 - *shiftF
    } else {
        shift = *shiftF
    }

    scanner := bufio.NewScanner(os.Stdin)
    for scanner.Scan() {
        cipher(scanner.Text(), shift)
    }
}

Type conversions

bufio.Scanner holds bytes internally: the doc for Scanner.Bytes says:

  The underlying array may point to data that will be overwritten by a subsequent call to Scan. It does no allocation.

while Scanner.Text indicates that it returns:

  a newly allocated string.

So here, you allocate a string to copy bytes; and then you allocate a new slice of type []byte to copy this string, which is also an expensive operation.

So you're saving two conversions by not dealing with strings at all:

func main() {
    …
    for scanner.Scan() {
        fmt.Printf("%s\n", cipher(scanner.Bytes(), shift))
    }
}

func cipher(bytes []byte, shift int) []byte {
    var line []byte
    for _, b := range bytes {
        if b >= 'A' && b <= 'Z' {
            b = byte((int(b-'A')+shift)%26 + 'A')
        } else if b >= 'a' && b <= 'z' {
            b = byte((int(b-'a')+shift)%26 + 'a')
        }
        line = append(line, b)
    }
    return line
}

Note the slight change in the way you print the result: using fmt.Println on a []byte variable will print the numeric value of each byte, something like [116 104 101 32 103 97 109 101], so you have to use fmt.Printf to tell it to print it like a string.

You're also wondering whether the conversions from byte to int are inefficient: kind of, yes. Arithmetic operations work on bytes, so you could do instead:

shiftB := byte(shift)
b = ((b-'A')+shiftB)%26 + 'A'

However, this is just a simple type conversion: the performance gain from it will be much lower than when avoiding memory allocations, so you shouldn't worry about it too much =)

Optimizations

Initializing a []byte variable and appending char by char to it is not great: the slice is going to be re-sized every time it hits its maximum default size. It's even worse in your code: you're initializing a string variable and appending char by char to it, which will reallocate the string each time. In cipher, you know in advance which size line is going to be: the same as the original slice. So you can initialize directly with the correct allocated capacity:

    line := make([]byte, 0, len(bytes))

okay, now let's go even further: calling append at each iteration means that under the hood, there will be a check to see if the slice over capacity. We know it's not going to happen, because the sizes are the same: why not always allocate the exact amount of memory that we need and write at the right place directly?

func cipher(bytes []byte, shift int) []byte {
    shiftB := byte(shift)
    line := make([]byte, len(bytes))
    for i, b := range bytes {
        if b >= 'A' && b <= 'Z' {
            b = ((b-'A')+shiftB)%26 + 'A'
        } else if b >= 'a' && b <= 'z' {
            b = ((b-'a')+shiftB)%26 + 'a'
        }
        line[i] = b
    }
    return line
}

We can go even further by noticing that the bytes argument won't be used after we call cipher. So we can re-use this slice instead of allocating a new one!

func cipher(bytes []byte, shift int) []byte {
    shiftB := byte(shift)
    for i, b := range bytes {
        // shift b…
        bytes[i] = b
    }
    return bytes // optional: callers could simply reuse the argument
}

Final words

The code we obtain is more readable, definitely more efficient; and has the same behavior as yours. However, there are still two potential issues with it:

Encoding: we're considering every byte of the input separately, as if it were ASCII; so it will probably not do what you want if the input is in UTF-8.
Input size: if the input has very long lines, then using bufio.NewScanner is a bad idea; see for example this SO question.


Symmetric decryption with javax.crypto I'm trying to determine if this code could be a memory hog with huge files. Can anyone tell me if there is anything I can do to make this more memory efficient, or is this how java.crypto works?

private byte[] symmetricDecryptAttachment(byte[] encryptedAttachmentBytes, byte[] keyBytes) throws IOException, GeneralSecurityException, EIPException {
    byte[] decryptedValue = null;

    // TODO: change to use non-deprecated method ConfigurationManager:getParsedStringValue() instead of getStringValue() which is deprecated
    String decryptionAlgorithm = manager.getStringValue(SymmetricDecryptionAlgorithm);
    String cipherBlockMode = manager.getStringValue(SymmetricDecryptionBlockCipherMode);
    String padding = manager.getStringValue(SymmetricDecryptionPadding);

    // Key bytes are only the first 16 bytes
    final int KEY_BYTES_STARTING_BYTE = 0;
    final int KEY_BYTES_ENDING_BYTE = 16;

    // IV bytes are only the first 16 bytes
    final int IV_BYTES_STARTING_BYTE = 0;
    final int IV_BYTES_ENDING_BYTE = 16;

    // Cipher bytes are everything after first 16 bytes (iv bytes)
    final int CIPHER_BYTES_STARTING_BYTE = IV_BYTES_ENDING_BYTE;

    try {
        // 1. Get the cipher ready to start doing the AES transformation
        Cipher cipher = Cipher.getInstance(decryptionAlgorithm + "/" + cipherBlockMode + "/" + padding);

        // 2. Only take the first 16 bytes so that it is 128 bits as we are doing AES 128 bit decryption
        //      *Note: if this is not 128 bit decryption then you might have to change this ...
        byte[] trimmedKeyBytes = Arrays.copyOfRange(keyBytes, KEY_BYTES_STARTING_BYTE, KEY_BYTES_ENDING_BYTE);

        // 3. Start the decryption process by creating a key spec object
        SecretKeySpec keySpec = new SecretKeySpec(trimmedKeyBytes, decryptionAlgorithm);

        // 4. The initialization vector (IV) is the first 16 bytes/octets of the cipher value in the body.
        // 4.1 To recover the IV, Base64 decode the CipherValue
        byte[] cipherBytes = Base64.decodeBase64(encryptedAttachmentBytes);

        // 4.2 The remainder after the first 16 bytes is the encrypted message
        //      -do +2 from start because we want to remove \r\n at the beginning and -2 to remove \r\n from the end
        // Remove the new line and carriage returns from beginning and end, if they are there
        byte[] encryptedBytes = null;
        byte[] ivBytes = null;
        if( encryptedAttachmentBytes[0] == '\r' &&
                encryptedAttachmentBytes[1] == '\n' &&
                encryptedAttachmentBytes[encryptedAttachmentBytes.length-2] == '\r' &&
                encryptedAttachmentBytes[encryptedAttachmentBytes.length-1] == '\n') {
            encryptedBytes = Arrays.copyOfRange(encryptedAttachmentBytes, CIPHER_BYTES_STARTING_BYTE + 2, encryptedAttachmentBytes.length - 2);

            // 4.2a And take bytes 2 thru 18 instead of 0 thru 16 to account for extra \r \n's
            ivBytes = Arrays.copyOfRange(encryptedAttachmentBytes, IV_BYTES_STARTING_BYTE + 2, IV_BYTES_ENDING_BYTE + 2);
        }else{
            encryptedBytes = Arrays.copyOfRange(encryptedAttachmentBytes, CIPHER_BYTES_STARTING_BYTE, encryptedAttachmentBytes.length);

            // 4.2b And take the first 16 bytes from encrypted text to get the IV bytes
            ivBytes = Arrays.copyOfRange(encryptedAttachmentBytes, IV_BYTES_STARTING_BYTE, IV_BYTES_ENDING_BYTE);
        }

        // 5. Create the IV Parameter spec from the ivBytes obtained above
        IvParameterSpec iv = new IvParameterSpec(ivBytes);

        // 6. Initialize the cipher using the key spec and iv we just initialized
        cipher.init(Cipher.DECRYPT_MODE, keySpec, iv);

        // 7. Finish the decryption process
        decryptedValue = cipher.doFinal(encryptedBytes);
    } catch (Exception e) {
        System.err.println("Exception occurred in symmetricDecryptAttachment() of SOAPDecryptionProcessor: \n" + e.getMessage());
        throw new EIPException(e.getMessage());
    }

    return decryptedValue;
}

String decryptionAlgorithm = manager.getStringValue(SymmetricDecryptionAlgorithm);

What manager is this, and why does the method have to perform its own configuration?

// Key bytes are only the first 16 bytes
final int KEY_BYTES_STARTING_BYTE = 0;
final int KEY_BYTES_ENDING_BYTE = 16;

final int IV_BYTES_STARTING_BYTE = 0;
final int IV_BYTES_ENDING_BYTE = 16;

Entirely not needed, and if they were they should be private static class variables, the Java equivalent of constants. What about using Cipher#getBlockSize()?

// 1. Get the cipher ready to start doing the AES transformation
Cipher cipher = Cipher.getInstance(decryptionAlgorithm + "/" + cipherBlockMode + "/" + padding);

The comment is dangerous, as it mentions AES but you configure the decryptionAlgorithm. I'm not sure why you would need 3 separate configuration fields for this.

byte[] trimmedKeyBytes = Arrays.copyOfRange(keyBytes, KEY_BYTES_STARTING_BYTE, KEY_BYTES_ENDING_BYTE);

So if the user supplies a 256 bit key, then just use the 128 bits and do not throw error? That's not good practice at all.

byte[] encryptedBytes = null;
byte[] ivBytes = null;

You are already assigning the references in the if statement, no need to initialize them to null. You can even make them final so that you know that each branch will initialize the variables!

//      -do +2 from start because we want to remove \r\n at the beginning and -2 to remove \r\n from the end
// Remove the new line and carriage returns from beginning and end, if they are there

You have got binary data, which has been base 64 decoded, and your originally binary ciphertext could maybe include a carriage return + line feed? Well, a random IV or ciphertext will probably contain that with a chance of 1/65536, which means that your decryption will fail once in about 4 billion times. You'll just have to wait long enough for it to happen.

encryptedBytes = Arrays.copyOfRange(encryptedAttachmentBytes, CIPHER_BYTES_STARTING_BYTE, encryptedAttachmentBytes.length);

First of all, I have no clue why you would copy the encrypted data and then the IV bytes, as that makes no sense at all - you never receive or need them in that order.

But note that both the IvParameterSpec() constructor and Cipher#doFinal() method have forms that already let you define an offset and amount of bytes to use. For the IV that doesn't matter much, for the ciphertext it adds insult to injury.

private byte[] symmetricDecryptAttachment(byte[] encryptedAttachmentBytes, byte[] keyBytes) throws IOException, GeneralSecurityException, EIPException {
    try {
        ...
    } catch (Exception e) {
        System.err.println("Exception occurred in symmetricDecryptAttachment() of SOAPDecryptionProcessor: \n" + e.getMessage());
        throw new EIPException(e.getMessage());
    }

Pokemon exception handling, using System.out, removing the exception from the stack trace, not specifying which exceptions can be thrown, no distinguishing between different types of exception (NoSuchAlgorithmException is different from a bad ciphertext, right?).

That kind of exception handling has to go.

Be warned that CBC is only useful for confidentiality, and then preferably only over authenticated messages or for in place encryption / decryption. I'm not sure if you're using that, but using GCM is probably a better option (within an IV of 12 bytes).

As for your question: yes, use streaming. Incremental updates are also possible, but given that your ciphertext is likely arriving per stream, using CipherInputStream makes most sense. Note that the Base64 encoder class can also be wrapped over a stream.


Extending the MVC Routecollection I'm currently developping an MVC framework on which I will be writing my websites. This is done through the usage of a starter kit.

In standard MVC, your routes are registered like the following:

/// <summary>
///     Registers the routes.
/// </summary>
/// <param name="routes">The <see cref="RouteCollection" /> to which the routes will be added.</param>
public static void RegisterRoutes(RouteCollection routes)
{
    routes.IgnoreRoute("{resource}.axd/{*pathInfo}");

    // All the required routes are placed here.
    routes.MapRoute("Website", "{controller}/{action}/{id}", new { controller = "Home", action = "Index", id = UrlParameter.Optional });
}

But since it's a framework, there's a need to add routes to the collection, so I started with an interface:

public interface IMvcRouteExtender
{
    #region Methods

    /// <summary>
    ///     Register routes for the application.
    /// </summary>
    /// <param name="routes">The <see cref="RouteCollection"/> that contains all the required routes.</param>
    void RegisterRoutes(RouteCollection routes);

    #endregion
}

Now, in an implementation class, I can do the following:

public class CustomRouteConfiguration : IMvcRouteExtender
{
    #region IMvcRouteExtender

    /// <summary>
    ///     Registers the routes.
    /// </summary>
    /// <param name="routes">The <see cref="RouteCollection" /> to which the routes will be added.</param>
    public void RegisterRoutes(RouteCollection routes)
    {
        routes.MapRoute("Default", "pensions/save-and-pension", new { controller = "Home", action = "Index", id = UrlParameter.Optional });
    }

    #endregion
}

In order to be sure there available, the original routeconfig is changed so that it searches for those interfaces and register them.

/// <summary>
///     Registers the routes.
/// </summary>
/// <param name="routes">The <see cref="RouteCollection" /> to which the routes will be added.</param>
public static void RegisterRoutes(RouteCollection routes)
{
    routes.IgnoreRoute("{resource}.axd/{*pathInfo}");

    // Get all the types in the references assembly that implement the 'IMvcRouteExtender' interface.
    var types = AppDomain.CurrentDomain.GetAssemblies().ToList()
                .SelectMany(a => a.GetTypes())
                .Where(t => typeof(IMvcRouteExtender).IsAssignableFrom(t) && !t.IsInterface);

    // Register all the routes.
    types.Select(type => Activator.CreateInstance(type) as IMvcRouteExtender).Each(x => x.RegisterRoutes(routes));

    // All the required routes are placed here.
    routes.MapRoute("Website", "{controller}/{action}/{id}", new { controller = "Home", action = "Index", id = UrlParameter.Optional });
}

As you'll see above, I'm using reflection to load the types in which the routes are defined. This is working fine, but I don't like the overhead of using reflection.

Is there anyone who has an idea on how to make this more efficient? AKA, remove the dynamic type loading?

Since I'm writing a framework I cannot have a reference between routeConfig and CustomRouteConfig.

Also: Don't look at code semantics here, I'm just talking about a general idea to make the code more performant.

It was more or less worked out in the comments that the use of reflection is a fine way to go about it and I agree with that. What I can't understand is your use of regions. In IMvcRouteExtender you have one that wraps a single method. In my opinion, it just clutters the code. In your implementation, you have a #region IMvcRouteExtender. This is a little stickier. I could definitely see a benefit to this if your class was implementing multiple interfaces and you had a region for each interface, but I would remove it for the code here. Clutter like this distracts us from the code.

Some of your instructions also get kind of long. I'd shorten then up a bit with a few line breaks.

/// <summary>
///     Registers the routes.
/// </summary>
/// <param name="routes">The <see cref="RouteCollection" /> to which the routes will be added.</param>
public static void RegisterRoutes(RouteCollection routes)
{
    routes.IgnoreRoute("{resource}.axd/{*pathInfo}");

    // Get all the types in the references assembly that implement the 'IMvcRouteExtender' interface.
    var types = AppDomain.CurrentDomain.GetAssemblies()
                .ToList()
                .SelectMany(a => a.GetTypes())
                .Where(t => typeof(IMvcRouteExtender).IsAssignableFrom(t) && !t.IsInterface);

    // Register all the routes.
    types.Select(type => Activator.CreateInstance(type) as IMvcRouteExtender)
                .Each(x => x.RegisterRoutes(routes));

    // All the required routes are placed here.
    routes.MapRoute(
            "Website",
            "{controller}/{action}/{id}",
            new
            {
            controller = "Home",
            action = "Index",
            id = UrlParameter.Optional
            });
}

It's certainly not ideal still, but I prefer vertical scroll to horizontal scroll. It lets us see what the method is doing "all at once".


Convert Improper Fraction to Mixed Number with JavaScript My function converts an improper fraction to a mixed number (it does not AND should not simplify the fraction).

// Improper fraction to mixed number
// n = numerator
// d = denominator
// i = number
function improperFractionToMixedNumber(n, d) {
    i = (n / d) >> 0;

    n -= i * d;

    return [i, n, d];
}

// Convert 14/4 to mixed number
improperFractionToMixedNumber(14, 4); // 3, 2, 4

Can this be improved or simplified?

I don't care for this part:

i = (n / d) >> 0;

But it's the only way I could figure out how to see how many times a number equally divides into another number.

What you're doing with i = (n / d) >> 0 is a sign-propagating right shift. The interesting this is, you're doing a right shift by 0 (I'd imagine because you'd otherwise get NaN. Your method of doing so is a cheap version (read: performance hack?) of doing i = parseInt(n / d). Here's your function with the SPRS removed and replaced with parseInt() instead. I've also implemented simplification.

function improperFractionToMixedNumber(n, d) {
    i = parseInt(n / d);
    n -= i * d;
    return [i, n, d];
}

Here is the same function, but also implementing simplification via a helper function reduce:

// Improper fraction to mixed number
// n = numerator
// d = denominator
// i = number
function improperFractionToMixedNumber(n, d) {
    i = parseInt(n / d);
    n -= i * d;
    return [i, reduce(n,d)];
}

function reduce(numerator,denominator){
    if (isNaN(numerator) || isNaN(denominator))
      return NaN;
    var gcd = function gcd(a, b){ return b ? gcd(b, a%b) : a; };
    gcd = gcd(numerator, denominator);
    return [numerator/gcd, denominator/gcd];
}


Shortening method based on an argument name As you can see below, I have a method which executes statements based on the first letter of a component firing an ItemEvent:

public void itemStateChanged(ItemEvent ie) {
    if(ie.getSource() == rRadioButton) {
        currentScale = "r";
        ((DefaultEditor) rSpinner.getEditor()).getTextField().requestFocus();
    }
    else if(ie.getSource() == gRadioButton) {
        currentScale = "g";
        ((DefaultEditor) gSpinner.getEditor()).getTextField().requestFocus();
    }
    else if(ie.getSource() == bRadioButton) {
        currentScale = "b";
        ((DefaultEditor) bSpinner.getEditor()).getTextField().requestFocus();
    }
    //...
}

Is it possible to easily shorten the method and make it more automatic?

If the argument was an ActionEvent I could use getActionCommand to assign the value for currentScale but the same number of lines of code would be needed first to setActionCommand for every component.

I could also use setName and getName for components but this will result it the same issue as described above.

Also it would be good to be able to automatically point to a specific Spinner based on a specific RadioButton as you can see in the attached source code.

Maybe there is a possibility to use reflection for this but I don't know if this could work.

Without seeing the greater context in which this code appears such as how the components are constructed, and not having kept up with Spring since 1999, I can only address the immediate code. The above could be converted to use two maps: one for the text fields and another for the currentScale strings.

In the code that builds the components, create the two maps and store them in instance fields of the same class.

private final Map<Object, JTextField> textFieldsByRadioButton = new IdentityHashMap<>();
private final Map<Object, String> scalesByRadioButton = new IdentityHashMap<>();

private void buildComponents() {
    ...
    addRadioButtonSpinnerMapping(rRadioButton, rSpinner, "r");
    addRadioButtonSpinnerMapping(gRadioButton, gSpinner, "g");
    addRadioButtonSpinnerMapping(bRadioButton, bSpinner, "b");
}

private void addRadioButtonSpinnerMapping(JRadioButton button, JSpinner spinner, String scale) {
    textFieldsByRadioButton.put(button, ((DefaultEditor) spinner.getEditor()).getTextField());
    scalesByRadioButton.put(button, scale);
}

public void itemStateChanged(ItemEvent ie) {
    Object source = ie.getSource();
    JTextField textField = textFieldsByRadioButton.get(source);
    if (textField != null) {
        currentScale = scalesByRadioButton.get(source);
        textField.requestFocus();
    }
    ...
}

While it requires slightly more code, it only takes one line to add another spinner.