23

@vnp's code is solid and helpful, but his stringify_state_helper is a single-purpose function, and still leaves a degree of repetition and memory management in stringify_state. I'd rather have general-purpose to_string that takes printf-style arguments, allocates sufficient space for the converted result, and prints into that space, and returns the result: #...


12

First of all, I agree with others, you could replace lines var size = 4; var requiredSize = 8; with const int size = sizeof(int); const int requiredSize = sizeof(long); This removes magic values from the code. Secondly, your code is hard to understand. For instance: result[i] = (byte)((value & ((ulong)0xFF << bitOffset)) >> bitOffset); ...


12

I have some ideas about how you might be able to improve your program. Avoid problems Rather than trying to deal with the problem for every instruction, one approach is avoiding it entirely. One way to do that is to simply append a number of bytes to the end of the vector. If the maximum bytes for an instruction is \$n\$, then append \$n-1\$ bytes to the ...


11

encoding/binary package may have what you need. Check this: http://golang.org/pkg/encoding/binary/#example_Read Your code with modified read_int32 function could be: package main import ( "bytes" "encoding/binary" "fmt" ) func read_int32(data []byte) (ret int32) { buf := bytes.NewBuffer(data) binary.Read(buf, binary.LittleEndian, &...


11

Here are a few remarks. I'm not perfect or write perfect code but I hope these tips will help write better code: Naming consistency: It would be easier to understand if the names of your two methods had opposing names. For example: SerializeToFile and DeserializeFromFile. It's obvious that deserialization will return an object, omit that in the name. Also,...


11

I don't think there is a built-in method. Yours is pretty good but we could make some improvements: Parameters should be camelCase => hexString. You should favour StringBuilder when building up strings. You should step through the string in increments of 2 to cut down on the maths. You should validate the argument. You should prefer var when the type is ...


11

Although you have done a decent job, you could do this in a better way, without even to use unsafe. First thing to mention, you should always use braces {} although they are optional for single lined if statements. This will just make your code less error prone which is in such a security context wanted. Instead of using if (encoding==null) { encoding = ...


11

Comments You have #Player Class and #Enemy Class as comments. Comments should be used to clarify something that may otherwise be unclear. The line class Player: means that you are creating the Player class, so that comment does nothing. Spacing You need a little whitespace. Between the methods in the Player class, for example, I would add a blank line. ...


11

Consider factoring it out into a function: static size_t stringify_state_helper(State * state, char * buf) { return snprintf(buf, ....); } char * stringify_state(State * state) { size_t len = stringify_state_helper(state, NULL); char * buf = malloc(len + 1); if (buf) { stringify_state_helper(state, buf); } return buf; }


10

Your methodology is solid on the generics front. Highly recommend using using statements rather than try..finallys. I also converted the methods to extension methods. namespace Codingoutloud { using System.IO; using System.Runtime.Serialization.Formatters.Binary; public static class ByteArraySerializer { public static byte[] Serialize<...


10

Avoid single letter variable names. Spell out the words they stand for to make your code easier to read. Your comments are useless. You are just restating the code in less detail. The only comment you should have is explaining about the '=' like you did in your post. Don't double-space your code. Pickle isn't safe. You can construct a pickle that does ...


10

This code is begging for an EnumMap as the data store, rather than the individual variables. Creating an Enum with values like: public enum OrganizationField {ACCOUNT_NUMBER, ZIP_CODE, ....}; In your Organization you would have: private final EnumMap<OrganizationField,String> fieldMap = new ...; and methods like: public String getAccountNumber() {...


10

A few points tickle: Why is it a static method? None of the comments are helpful. Good comments say why, not what. I'd remove them all. Bracing is inconsistent. Consider: if (!(dr[propertyInfo.Name] is DBNull)) propertyInfo.SetValue(ob, dr[propertyInfo.Name]); And then: if (!(dr[primaryKey] is DBNull)) { propertyInfo.SetValue(ob, dr[primaryKey]....


10

DRY Let's start with your methods arsenal(), astonVilla() and so on. From the title of the question, I think you're already aware these are problematic. Remember that while at its core your program is going to be made up of branches and loops, high-level languages like Java allow us to write in a way that the source code captures the structure of the task ...


10

I don't find many things to complain about :-) but this bothers me... I would change the name of those variables: var requiredSize = 4; var size = 4; to something like var intSize = sizeof(int); or var numOfInt32Bytes = sizeof(int); It might be the required size but what size is it actually? The code doesn't tell us this. This method and it's t are ...


10

I've just got a few little tips for this tiny little program: Use consistent spacing. You mostly have a = b, but in one place you have a =b. Probably just a typo, but worth mentioning. In Ruby, you have string interpolation, which (in my experience) tends to be preferred to printf. puts " " is equivalent to puts, at least the way you're using it. a, b, c, ...


10

One things for certain... an array of tuples which is basically being used as a glorified dictionary is never going to suffice as an acceptable data model. Moreover, an array of bytes isn't really that much useful than what we start with. So what's clear, we need an actual data model. So names and data types may need some twiddling, but here's what I ...


10

readonly will not make members of your static serializers readonly. While you cannot reassign another serializer to replace it, its members can still be modified. Since you have access to C# you can use a get-only property to return a new instance : public static DelimitedSerializer TsvSerializer => new DelimitedSerializer { ColumnDelimiter = "\t", ...


10

Not bad! Let's go through it line-by-line. typedef unsigned char BYTE; This is fine, but just so you know, starting from C++17, there is a std::byte that could replace this. If you're not using C++17 yet, fine, but if this is code is for C++17 or better, that's something you can take advantage of. inline int bmpEncoder(const std::string &location, ...


9

Typedef your structs: Personally, I don't like having to write struct every time when using a user defined type: struct tlv xyz; I would suggest that you use a typedef for your structured types: typedef struct tlv { int8_t type; // type uint8_t * data; // pointer to data int16_t size; // size of data } tlv_t; typedef struct tlv_chain { ...


9

The method does two different things and thus should be split in two: Interpret a hex string as a sequence of bytes. You can find many possible implementations at How do you convert Byte Array to Hexadecimal String, and vice versa?. Yours has quadratic runtime (due to the string concatenation pattern RobH noted) and creates a new string object for each ...


9

The GNU libc already provides asprintf, which avoids exactly the repetition you are concerned about. I haven't looked at the solutions of other operating systems (Windows, BSD, AIX, Solaris, embedded), though I hope they provide something equivalent.


8

First of all, fundamentally: std::string is a data type to hold text. Exclusively. It should not be used to hold binary data. Use a std::vector<(unsigned) char> for that. Secondly, you are using heap allocation without needing to: T* pt = new T(t); This makes no sense at all, and introduces the potential of a memory leak. You could simply make a ...


8

How about this: To Save: Settings.Default.blEnabled = string.Join(",", blenabled.Select(x=>x?"1":"0")); To Load: if(!string.IsNullOrWhitespace(Settings.Default.blEnabled)) blenabled = Settings.Default.blEnabled .Split(',') .Select(x=>Convert.ToBoolean(int.Parse(x))) .ToList(); Is there some requirement that you save them as integers? ...


8

Before we start, take a look at this comment of yours. It's a prime example of why you shouldn't always trust the comments. 'This gets the text that comes before the : 'Example Company:TextArea would return TextArea. controlOption = textSelection.Substring(0, textSelection.IndexOf(":"c)) The text before : is in fact Company ;) Now, the name of the ...


8

Factory is a terrible name for a class. What does it make? As best as I can tell, it doesn't make anything, it just de/serializes. Honestly, I don't think you even need it; you could easily put (de)serialize in the respective classes. LineSerializer and LineDeserializer operate on Strings, which may very well contain newlines or carriage returns or both. ...


8

This has been thoroughly answered already I'd say, but I'll have a go regardless... You're loading 2 files (I don't know why, but I'll roll with it), and doing the same things with both. That calls for a method, or a loop, or both. I'm thinking something like this: %w(TESTFILE.txt TESTFILE2.txt).each.with_index do |path, i| data = File.read(path) hex =...


8

Bug The swap<T>() method isn't doing what you think it does, because the passed in T obj1, T obj2 aren't passed with the ref keyword the values ar only changed in that method not targeting the variable values of the calling method. So this int min = 10; int max = 5; swap(min, max); Console.WriteLine(String.Format("{0} : {1}", min, max); will ...


8

Given that this was for an interview I'll refrain from saying that Javadoc is missing. private static final Point closest = new Point(-200, 300); That's a violation of the Java Style Guidelines, constants are to be named UPPER_CAMELCASE. Not to mention that the name could be better. private static final Point CLOSEST_POINT = new Point(-200, 300); ...


8

I haven't used it, however all of the documentation seems to suggest that you need explicitly free the memory allocated through: IntPtr handle = Marshal.AllocHGlobal(bufferSize); With a paired call to: Marshal.FreeHGlobal(handle); You don't seem to need the handle after you've performed the marshalling, so it could be as simple as updating the function ...


Only top voted, non community-wiki answers of a minimum length are eligible